Page MenuHomePhabricator

[clangd] Add CMake option to (not) link in clang-tidy checks
ClosedPublic

Authored by sammccall on Jul 9 2021, 12:44 AM.

Details

Summary

This reduces the size of the dependency graph and makes incremental
development a little more pleasant (less rebuilding).

This introduces a bit of complexity/fragility as some tests verify
clang-tidy behavior. I attempted to isolate these and build/run as much
of the tests as possible in both configs to prevent rot.

Expectation is that (some) developers will use this locally, but
buildbots etc will keep testing clang-tidy.

Fixes https://github.com/clangd/clangd/issues/233

Diff Detail

Event Timeline

sammccall created this revision.Jul 9 2021, 12:44 AM
sammccall requested review of this revision.Jul 9 2021, 12:44 AM
kadircet added inline comments.Jul 9 2021, 2:16 AM
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
280

why do we need to disable this test? it doesn't really build an ast or assert on the diagnostics from clang-tidy in any way, seems to be purely about configs.

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
153–154

you probably want to drop this too ?

167

ah, this was actually checking for a particular change in the way we transform clang(-tidy) provided diag ranges to editor-friendly ones.

any diagnostic without any range info and containing a fix which is only insertion should be fine (one that i could find is for_range_dereference).

266

ah, should've kept reading before leaving the comment above. feel free to ignore that (or just drop this test and rely only on clang diagnostics for checking that behaviour?)

294–312

i think we should suppress this with -Wno-unsequenced (and get rid of the Else part). Moreover this is the only test requiring an Else bit (if I didn't miss any). WDYT about just having a new file TidyIntegrationTests or TidyDiagnosticTests and moving all of these there, or having two different sections in this file to only enable these tests when tidy is on. It would imply tests that want to check a mix of tidy and clang diagnostics needs to go into the tidy section and cannot be tested on non-tidy builds, but that sounds like the price we need to pay anyway, whereas introducing the ifTidyChecks complicates things a little more (IMO).

Will building with this option just prevent linking the clang-tidy checks into clangd, or will it also prevent building them in the first place?

In my experience, it's the building that's the primary compile time expense, so if the intention is to address https://github.com/clangd/clangd/issues/233, we would want to avoid building them as well.

sammccall marked 5 inline comments as done.Jul 13 2021, 5:49 AM

Will building with this option just prevent linking the clang-tidy checks into clangd, or will it also prevent building them in the first place?

If you're running ninja clangd or ninja check-clangd they aren't built anymore even if dirty. (Just tested this)
This change breaks the only build dependency path between clangd/ClangdTests and the clang-tidy checks.

Of course all/check-all/check-clang-tools will still build the clang-tidy checks.

clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
280

We have clever logic that verifies that check names (but not wildcards) in configs actually match real checks.

Put ifdefs around the relevant assertions instead, so it should be now self-explanatory.

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
266

This really belongs in the diagnostic ranges test, so switched to a different diag as you suggested.

294–312

i think we should suppress this with -Wno-unsequenced (and get rid of the Else part). Moreover this is the only test requiring an Else bit (if I didn't miss any).

Done, and ripped out Else support.

WDYT about just having a new file TidyIntegrationTests or TidyDiagnosticTests and moving all of these there, or having two different sections in this file to only enable these tests when tidy is on

I started with this but having substantial amounts of nontrivial code completely ifdef'd out makes me nervous about maintenance, as we'd lose all tooling support on these tests if we use this config.

This solution is significantly safer I think:

  • mostly preserves refactoring options even with clang-tidy off (e.g. local xrefs)
  • type checks all the matchers, even the clang-tidy ones with clang-tidy off
  • verifies that the code snippets in TestTU actually works
  • explicitly shows the difference with clang-tidy on vs off

ifTidyChecks complicates things a little more (IMO).

Agree. I think the benefits of the tests being live code outweigh the relatively minor complexity. Maybe there's a better tradeoff to be had though.

sammccall updated this revision to Diff 358241.Jul 13 2021, 5:49 AM
sammccall marked 3 inline comments as done.

Address review comments

kadircet accepted this revision.Jul 13 2021, 7:12 AM

thanks, lgtm!

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
266

nit: is this testing anything useful now? maybe just drop it?

This revision is now accepted and ready to land.Jul 13 2021, 7:12 AM
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.

Nit: I find the name a bit confusing. When I first saw this commit, I thought this would be a list of clang-tidy checks to support, not a binary on/off toggle.