Page MenuHomePhabricator

[clangd] Move clang-tidy check modifications into ClangdServer
ClosedPublic

Authored by kadircet on Jul 6 2020, 6:48 AM.

Details

Summary

This enables sharing the logic between standalone clangd and embedders
of it. The new approach should be same performance-wise, as it is only called
once per addDocument call.

This patch also introduces a blacklisting code path for disabling crashy or
high-noise tests, until we figure out a way to make them work with clangd-setup.

The biggest difference is the way we make use of preambles, hence those checks
can't see directives coming from the preamble section of the file. The second
thing is the fact that code might-not be compiling while clangd is trying to
build an AST, hence some checks might choke on those incomplete ASTs.

Diff Detail

Event Timeline

kadircet created this revision.Jul 6 2020, 6:48 AM
kadircet updated this revision to Diff 275709.Jul 6 2020, 7:41 AM
  • Add tests and also disable bugprone-use-after-move

Do you think there should be a hidden command line option to disable disabling blacklisted checks, purely to prevent hindering attempts to fix these problematic checks.

clang-tools-extra/clangd/ClangdServer.cpp
117

Return by StringRef?

nridge added a subscriber: nridge.Jul 21 2020, 4:16 PM

Do you think there should be a hidden command line option to disable disabling blacklisted checks, purely to prevent hindering attempts to fix these problematic checks.

I would expect those to be fixed and verified with unittests that are using TestTU, so the changes in ClangdServer shouldn't really be relevant to them. And if developer is trying to test something locally, i think it is better for them to just update the blacklist file(they are going to end up doing that anyways and perform possibly multiple compiles), so a flag to change blacklists behaviour might not be that meaningful.

njames93 added inline comments.Aug 11 2020, 5:33 PM
clang-tools-extra/clangd/ClangdServer.cpp
240

Should the ! be removed the branches be swapped? Just looks cleaner imo, WDYT?

kadircet added inline comments.Aug 12 2020, 3:38 AM
clang-tools-extra/clangd/ClangdServer.cpp
240

SGTM, will address once we've settled on the idea with Sam.

(and let this be a ping to him :D @sammccall )

aaron.ballman added inline comments.Aug 12 2020, 7:36 AM
clang-tools-extra/clangd/ClangdServer.cpp
117

How about getDisabledClangTidyChecks() (or literally any other name than blacklist)?

122

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).

129

Similarly, rename this. I'd suggest DisabledChecks.

kadircet added inline comments.Aug 12 2020, 11:21 AM
clang-tools-extra/clangd/ClangdServer.cpp
117

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?

aaron.ballman added inline comments.Aug 12 2020, 11:29 AM
clang-tools-extra/clangd/ClangdServer.cpp
117

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).

njames93 added inline comments.Aug 13 2020, 1:32 AM
clang-tools-extra/clangd/ClangdServer.cpp
122

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.

aaron.ballman added inline comments.Aug 13 2020, 5:40 AM
clang-tools-extra/clangd/ClangdServer.cpp
122

Ah, thank you for the explanation. Then how about UnusableWithinClangd or something other than FalsePositives for the name?

sammccall accepted this revision.Aug 13 2020, 7:25 AM
sammccall added inline comments.
clang-tools-extra/clangd/ClangdServer.cpp
115

nit: "returns a clang-tidy filter string: -check1,-check2"

117

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...

122

The whole list is of checks that aren't usable in clangd, FalsePositives is the specific reason. What's the problem with the name?

240

I'd actually write this explicitly as hasValue() since the nullopt vs empty distinction is critical here

243

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.

249

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)

257

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...)

clang-tools-extra/clangd/unittests/ClangdTests.cpp
1218

Maybe add a comment like "these checks don't work well in clangd, even if configured they shouldn't run"

This revision is now accepted and ready to land.Aug 13 2020, 7:25 AM
aaron.ballman added inline comments.Aug 13 2020, 7:32 AM
clang-tools-extra/clangd/ClangdServer.cpp
122

What's the problem with the name?

If this is a list of checks that should be disabled because they have a ton of false positives, I think the name is fine and the list is incomplete. If this is a list of checks that should be disabled because they're not well-supported in clangd specifically, I think the name is misleading and the list is fine.

sammccall added inline comments.Aug 13 2020, 7:39 AM
clang-tools-extra/clangd/ClangdServer.cpp
122

If this is a list of checks that should be disabled because they're not well-supported in clangd specifically, I think the name is misleading and the list is fine.

Fair enough - I think there's enough context to not need to repeat "in clangd" here though. This code is in clangd, in a function with a comment indicating that it's for checks that don't work well specifically in clangd. (Maybe s/not suitable to be run through clangd/don't work well in clangd/ though).

aaron.ballman added inline comments.Aug 13 2020, 7:46 AM
clang-tools-extra/clangd/ClangdServer.cpp
122

Fair enough - I think there's enough context to not need to repeat "in clangd" here though. This code is in clangd, in a function with a comment indicating that it's for checks that don't work well specifically in clangd. (Maybe s/not suitable to be run through clangd/don't work well in clangd/ though).

That makes sense to me.

kadircet marked 19 inline comments as done.Aug 13 2020, 8:47 AM
kadircet added inline comments.
clang-tools-extra/clangd/ClangdServer.cpp
117

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).

That's what we do when working in llvm and clang, but convention around clangd is putting implementation details in anon namespaces. I would rather not stray from it here.

As for the general naming issues, i went with UnusableTidyChecks.

249

SG. Just as a note, we were already doing this at every request, but indirectly through GetClangTidyOptions()

kadircet updated this revision to Diff 285389.Aug 13 2020, 8:47 AM
kadircet marked an inline comment as done.
  • Address comments
aaron.ballman added inline comments.Aug 13 2020, 8:49 AM
clang-tools-extra/clangd/ClangdServer.cpp
117

That's what we do when working in llvm and clang, but convention around clangd is putting implementation details in anon namespaces. I would rather not stray from it here.

It's unfortunate that clangd decided to stray from the community standards like this, but thank you for letting me know -- I agree it should match the local style.

This revision was landed with ongoing or failed builds.Aug 13 2020, 9:34 AM
This revision was automatically updated to reflect the committed changes.