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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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