kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:280
+#if CLANGD_TIDY_CHECKS
TEST_F(ConfigCompileTests, Tidy) {
----------------
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.
================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:151
auto TU = TestTU::withCode(Test.code());
TU.ClangTidyProvider = addTidyChecks("google-explicit-constructor");
EXPECT_THAT(
----------------
you probably want to drop this too ?
================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:167
- "change 'fod' to 'foo'"))),
- // We make sure here that the entire token is highlighted
- AllOf(Diag(Test.range("constructor"),
----------------
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`).
================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:259
+ class T{$explicit[[]]$constructor[[T]](int a);};
+
----------------
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?)
================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:301
+ "macro 'SQUARE' defined here"))),
Diag(Test.range("macroarg"),
+ "multiple unsequenced modifications to 'y'"),
----------------
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).
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
