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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
