[PATCH] D138505: [clangd] Don't run slow clang-tidy checks

[Sam McCall via Phabricator via cfe-commits]

sammccall planned changes to this revision.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:518
+
+    if (F.SlowChecks.has_value())
+      Out.Apply.push_back([V = **F.SlowChecks](const Params &, Config &C) {
----------------
kadircet wrote:
> nit: drop `has_value`?
This is a boolean, so there's an obvious+wrong interpretation of that version.
I think `has_value`, uh, has value here.


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:286
 
 bool isRegisteredTidyCheck(llvm::StringRef Check) {
   assert(!Check.empty());
----------------
kadircet wrote:
> should we update callers here to also emit a warning when they're turning on a slow check (and possibly mention SlowChecks override?) this might as well be our way of testing this to some extent. we'd still rely on a certain check-name always being part of the list (and pick a new element whenever we're updating the list), but at least we wouldn't rely on semantics of the check (i.e. also have a test case that'd trigger the warning).
Callers here == diagnostics for config files that name unknown checks.
I can add a warning for naming a slow check too, that makes sense.

> this might as well be our way of testing this to some extent

Yeah, `ASSERT_FALSE(isFastCheck("misc-const-completeness"))` and a corresponding positive case has some value even if we have to trivially update it sometimes.

I'm less sure about setting up a complicated end-to-end case that we'll have to rewrite/delete if the list changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138505