[PATCH] D83224: [clangd] Move clang-tidy check modifications into ClangdServer

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:113
+
+// Set of clang-tidy checks that are not suitable to be run through clangd,
+// either due to crashes or false positives.
----------------
nit: "returns a clang-tidy filter string: -check1,-check2"


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:115
+// either due to crashes or false positives.
+const char *getClangTidyBlacklist() {
+  static const std::string FalsePositives =
----------------
aaron.ballman wrote:
> kadircet wrote:
> > aaron.ballman wrote:
> > > njames93 wrote:
> > > > Return by StringRef?
> > > How about `getDisabledClangTidyChecks()` (or literally any other name than blacklist)?
> > thanks for bringing this to my attention, i will try to be more conscious next time.
> > 
> > I would prefer allow/deny as `disabled` might also be offensive in some contexts. Do you know if we already have some settlements around this one in the wider community?
> > thanks for bringing this to my attention, i will try to be more conscious next time.
> 
> No worries!
> 
> > I would prefer allow/deny as disabled might also be offensive in some contexts. Do you know if we already have some settlements around this one in the wider community?
> 
> I don't believe there's any consensus around avoiding use of "disabled" (we use the term in a lot of places, especially when paired with "enabled"), but I'd also be fine with allow/deny terminology instead.
> 
> As a minor drive-by comment, the function should also be marked `static` and placed outside of the anonymous namespace (per our usual coding style).
I dislike "disabled" because it's vague: every check that's not enabled is disabled, but only a few of them are mentioned here.

I'd suggest BadlyBehavedTidyChecks or UnusableTidyChecks...


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:120
+                       // clangd doesn't replay those when using a preamble.
+                       "-llvm-header-guard");
+  static const std::string CrashingChecks =
----------------
aaron.ballman wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > I suspect there are more checks that should be added here. For instance, much of `modernize-` is purely stylistic so it's easy to view as being false positives (like `modernize-use-trailing-return-types` or whatever it's called).
> > I don't think that's what this list is for. This seems to be purely for checks that don't run properly or crash inside clangd. `modernize-use-trailing-return-types` is a very noisy check but that's how it is when ran as normal clang-tidy. 
> Ah, thank you for the explanation. Then how about `UnusableWithinClangd` or something other than `FalsePositives` for the name?
The whole list is of checks that aren't usable in clangd, FalsePositives is the specific reason. What's the problem with the name?


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:210
         GetClangTidyOptions(*TFS.view(/*CWD=*/llvm::None), File);
+  if (!Opts.ClangTidyOpts.Checks) {
+    // If the user hasn't configured clang-tidy checks at all, including
----------------
kadircet wrote:
> njames93 wrote:
> > Should the `!` be removed the branches be swapped? Just looks cleaner imo, WDYT? 
> SGTM, will address once we've settled on the idea with Sam.
> 
> (and let this be a ping to him :D @sammccall )
I'd actually write this explicitly as hasValue() since the nullopt vs empty distinction is critical here


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:213
+    // via .clang-tidy, give them a nice set of checks.
+    // (This should be what the "default" options does, but it isn't...)
+    //
----------------
this comment no longer belongs here, it's to do with the structure of the various ClangTidyOptionsProvider implementations, which aren't visible here. Fine to just drop it.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:219
+    //  - being reasonably efficient
+    Opts.ClangTidyOpts.Checks = llvm::join_items(
+        ",", "readability-misleading-indentation",
----------------
if you're going to do this on every request, might as well pull out the default set of checks into a function getDefaultTidyChecks() or so.

(So it's just joined once, but more importantly so we're consistent in how we separate the mechanism vs policy)


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:227
+  } else {
+    // If user has enabled some checks, make sure clangd incompatible ones are
+    // disabled.
----------------
nit: they haven't necessarily enabled checks (e.g. they could have specified `-checks=""` on the command line).

If the set of checks was configured?

(This is kind of a nit but did confuse me...)


================
Comment at: clang-tools-extra/clangd/unittests/ClangdTests.cpp:1145
+    auto Opts = tidy::ClangTidyOptions::getDefaults();
+    Opts.Checks = "bugprone-use-after-move,llvm-header-guard";
+    return Opts;
----------------
Maybe add a comment like "these checks don't work well in clangd, even if configured they shouldn't run"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83224