[libcxx-commits] [PATCH] D131963: [libc++] Add custom clang-tidy checks

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:5
+set(SOURCES
+    robust_against_adl.cpp
+    libcpp_module.cpp)
----------------
ldionne wrote:
> Does this render our other tests for robustness against ADL obsolete? If so, we should take one of those, make it regress, and confirm that this clang-tidy check will catch it. If it doesn't, we should understand why.
> 
> Basically, I'd like us to have a clear understanding of whether we need to write manual tests for ADL robustness or not.
I think we should do that in a follow-up where we remove the current ADL tests and make sure clang-tidy catches all the cases. If clang-tidy doesn't catch something we should find out why and fix the clang-tidy check.


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:13-15
+  set(CMAKE_CXX_STANDARD 20)
+
+  project(cxx-tidy)
----------------
ldionne wrote:
> I'm not sure why we'd need those. We already set that at the top-level.
We use
```
set_target_properties(${target} PROPERTIES
    CXX_STANDARD 20
    CXX_STANDARD_REQUIRED YES
    CXX_EXTENSIONS NO)
```
at the top level. Should I do the same here?


================
Comment at: libcxx/test/tools/clang_tidy_checks/CMakeLists.txt:24
+                            )
+  set_target_properties(cxx-tidy PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
+  set(CMAKE_SHARED_MODULE_SUFFIX_CXX .dylib)
----------------
ldionne wrote:
> Based on the CMake documentation, this should already be the default. I don't understand why this is needed -- can we try removing it and see how it fails?
When I remove it the output file is set as `/home/nikolask/llvm-projects/default/build/lib/libcxx-tidy.plugin`. I don't know why it doesn't put it in the normal location.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131963