[PATCH] D138491: [clangd] Add script to maintain list of fast clang-tidy checks

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/TidyFastChecks.py:31
+    default='clang-tools-extra/clangd/TidyFastChecks.inc')
+parser.add_argument('--file', help='clangd binary to invoke',
+    default='clang/lib/Sema/Sema.cpp')
----------------
`file to use for benchmark`?


================
Comment at: clang-tools-extra/clangd/TidyFastChecks.py:41
+def read_old_fast(path):
+    text = subprocess.check_output(["cpp", "-P", "-FAST(C,T)=C", path])
+    for line in text.splitlines():
----------------
what does `-P` do? shouldn't the latter be `-DFAST`?


================
Comment at: clang-tools-extra/clangd/TidyFastChecks.py:70
+print("""// This file is generated, do not edit it directly!
+// This describes 
+#ifndef FAST
----------------
comment seems to be incomplete here


================
Comment at: clang-tools-extra/clangd/TidyFastChecks.py:70
+print("""// This file is generated, do not edit it directly!
+// This describes 
+#ifndef FAST
----------------
kadircet wrote:
> comment seems to be incomplete here
it might be useful to include name of the file benchmarks were performed on in the output.


================
Comment at: clang-tools-extra/clangd/TidyFastChecks.py:83
+    print(f"{decision} {check} {time}% <= {threshold}%", file=sys.stderr)
+    print(f"{decision}({check}, {time})")
+
----------------
i don't see the point in including delta in the output if we're also making the decision here. is it mostly for debugging purposes? e.g. when updating the list we get to see the difference?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138491/new/

https://reviews.llvm.org/D138491