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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 5 inline comments as done.
sammccall added a comment.

In D105679#2873109 <https://reviews.llvm.org/D105679#2873109>, @nridge wrote:

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



================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:280
 
+#if CLANGD_TIDY_CHECKS
 TEST_F(ConfigCompileTests, Tidy) {
----------------
kadircet wrote:
> 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.
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.


================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:259
 
+    class T{$explicit[[]]$constructor[[T]](int a);};
+
----------------
kadircet wrote:
> 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?)
This really belongs in the diagnostic ranges test, so switched to a different diag as you suggested.


================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:301
+                                  "macro 'SQUARE' defined here"))),
               Diag(Test.range("macroarg"),
+                   "multiple unsequenced modifications to 'y'"),
----------------
kadircet wrote:
> 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).
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105679