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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

i can't think of a proper way to test this out either. unless we somehow let slow-tidy-check list to be configurable, so probably fine to make sure it works locally and hope that new people introducing tidy checks do complain.



================
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) {
----------------
nit: drop `has_value`?


================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:286
 
 bool isRegisteredTidyCheck(llvm::StringRef Check) {
   assert(!Check.empty());
----------------
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).


================
Comment at: clang-tools-extra/clangd/tool/Check.cpp:468
+      // is counterproductive! 
+      if (CheckTidyTime.getNumOccurrences())
+        F.Diagnostics.ClangTidy.SlowChecks = true;
----------------
sammccall wrote:
> njames93 wrote:
> > How about changing this provide to always enable slow checks, but only use the provider if the flag is passed?
> This doesn't seem like it would simplify the code, but it does mean that if --check wants to override other config options then we'd need to add a second provider.
i think it's better to always have the provider, in case we decide to override more options later on (it'd be nice if we didn't come up with a new provider for every option we override). but seems fine either way for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138505