Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++][CI] Reenables clang-tidy. #90077

Merged
merged 1 commit into from
May 8, 2024

Conversation

mordante
Copy link
Member

@mordante mordante commented Apr 25, 2024

The patch does several things:

  • fixes module exports
  • disables clang-tidy with Clang-17 due to known issues
  • disabled clang-tidy on older libstdc++ versions since it lacks C++20 features used
  • fixes the CMake dependency

The issue why clang-tidy was not used in the CI was the last issue; the plugin was not a
dependency of the tests. Without a plugin the tests disable clang-tidy.

This was noticed while investigating #89898

@mordante mordante force-pushed the review/reenables_clang_tidy_in_ci branch 3 times, most recently from 50cd73a to 9dfaf1a Compare May 1, 2024 17:56
@mordante mordante force-pushed the review/reenables_clang_tidy_in_ci branch from 9dfaf1a to aa593ba Compare May 4, 2024 16:39
Copy link

github-actions bot commented May 4, 2024

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:
darker --check --diff -r 1a2a1fbd7c03381fe5e4f459f7081bef13366ef4...0bac7e0819fcfa767ee1447469c92b6138e3eb6a libcxx/test/libcxx/clang_tidy.gen.py
View the diff from darker here.
--- clang_tidy.gen.py	2024-05-08 15:24:03.000000 +0000
+++ clang_tidy.gen.py	2024-05-08 15:26:41.250489 +0000
@@ -16,11 +16,12 @@
 import sys
 sys.path.append(sys.argv[1])
 from libcxx.header_information import lit_header_restrictions, public_headers
 
 for header in public_headers:
-  print(f"""\
+    print(
+        f"""\
 //--- {header}.sh.cpp
 
 // REQUIRES: has-clang-tidy
 
 // The GCC compiler flags are not always compatible with clang-tidy.
@@ -34,6 +35,7 @@
 // TODO: run clang-tidy with modules enabled once they are supported
 // RUN: %{{clang-tidy}} %s --warnings-as-errors=* -header-filter=.* --checks='-*,libcpp-*' --load=%{{test-tools-dir}}/clang_tidy_checks/libcxx-tidy.plugin -- %{{compile_flags}} -fno-modules
 // RUN: %{{clang-tidy}} %s --warnings-as-errors=* -header-filter=.* --config-file=%{{libcxx-dir}}/.clang-tidy -- -Wweak-vtables %{{compile_flags}} -fno-modules
 
 #include <{header}>
-""")
+"""
+    )

@mordante mordante force-pushed the review/reenables_clang_tidy_in_ci branch from aa593ba to 957efd8 Compare May 4, 2024 18:59
Copy link

github-actions bot commented May 4, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@mordante mordante marked this pull request as ready for review May 7, 2024 15:25
@mordante mordante requested a review from a team as a code owner May 7, 2024 15:25
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label May 7, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 7, 2024

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

This is currently a test patch; locally clang-tidy seems to be used in the Docker image.

This was noticed while investigating #89898


Full diff: https://github.com/llvm/llvm-project/pull/90077.diff

5 Files Affected:

  • (modified) libcxx/modules/std/chrono.inc (+5-1)
  • (modified) libcxx/modules/std/ranges.inc (+4-3)
  • (modified) libcxx/test/CMakeLists.txt (+6)
  • (modified) libcxx/test/libcxx/clang_tidy.gen.py (+3)
  • (modified) libcxx/test/tools/clang_tidy_checks/CMakeLists.txt (+20-2)
diff --git a/libcxx/modules/std/chrono.inc b/libcxx/modules/std/chrono.inc
index 1265e21dc54ef..aa275a112e0f0 100644
--- a/libcxx/modules/std/chrono.inc
+++ b/libcxx/modules/std/chrono.inc
@@ -213,11 +213,15 @@ export namespace std {
     using std::chrono::ambiguous_local_time;
     using std::chrono::nonexistent_local_time;
 #    endif // if 0
+#endif   //  !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) &&
+         //  !defined(_LIBCPP_HAS_NO_LOCALIZATION)
 
     // [time.zone.info], information classes
     using std::chrono::local_info;
     using std::chrono::sys_info;
 
+#if !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) &&                              \
+    !defined(_LIBCPP_HAS_NO_LOCALIZATION)
 #    if 0
     // [time.zone.timezone], class time_zone
     using std::chrono::choose;
@@ -248,7 +252,7 @@ export namespace std {
 #    endif
 #  endif // _LIBCPP_ENABLE_EXPERIMENTAL
 #endif   //  !defined(_LIBCPP_HAS_NO_TIME_ZONE_DATABASE) && !defined(_LIBCPP_HAS_NO_FILESYSTEM) &&
-         //    !defined(_LIBCPP_HAS_NO_LOCALIZATION)
+         //  !defined(_LIBCPP_HAS_NO_LOCALIZATION)
 
   } // namespace chrono
 
diff --git a/libcxx/modules/std/ranges.inc b/libcxx/modules/std/ranges.inc
index 80f31c79a1a40..f71efe948ede1 100644
--- a/libcxx/modules/std/ranges.inc
+++ b/libcxx/modules/std/ranges.inc
@@ -138,9 +138,6 @@ export namespace std {
     }
 #endif // _LIBCPP_HAS_NO_LOCALIZATION
 
-#if _LIBCPP_STD_VER >= 23
-    // [range.adaptor.object], range adaptor objects
-    using std::ranges::range_adaptor_closure;
     // Note: This declaration not in the synopsis or explicitly in the wording.
     // However it is needed for the range adaptors.
     // [range.adaptor.object]/3
@@ -151,7 +148,11 @@ export namespace std {
     //   involving an object of type cv D as an operand to the | operator is
     //   undefined if overload resolution selects a program-defined operator|
     //   function.
+    // This is used internally in C++20 mode.
     using std::ranges::operator|;
+#if _LIBCPP_STD_VER >= 23
+    // [range.adaptor.object], range adaptor objects
+    using std::ranges::range_adaptor_closure;
 #endif
 
     // [range.all], all view
diff --git a/libcxx/test/CMakeLists.txt b/libcxx/test/CMakeLists.txt
index e0d3a0dbc4003..fd57aa9fe8b37 100644
--- a/libcxx/test/CMakeLists.txt
+++ b/libcxx/test/CMakeLists.txt
@@ -1,5 +1,11 @@
 include(HandleLitArguments)
 add_subdirectory(tools)
+# When the tools add clang-tidy support, the dependencies need to be updated.
+# This cannot be done in the tools CMakeLists.txt since that does not update
+# the status in this (a parent) directory.
+if(TARGET cxx-tidy)
+  list(APPEND LIBCXX_TEST_DEPS cxx-tidy)
+endif()
 
 # By default, libcxx and libcxxabi share a library directory.
 if (NOT LIBCXX_CXX_ABI_LIBRARY_PATH)
diff --git a/libcxx/test/libcxx/clang_tidy.gen.py b/libcxx/test/libcxx/clang_tidy.gen.py
index 19b6a999df607..8419a6f4637c5 100644
--- a/libcxx/test/libcxx/clang_tidy.gen.py
+++ b/libcxx/test/libcxx/clang_tidy.gen.py
@@ -24,6 +24,9 @@
 // The GCC compiler flags are not always compatible with clang-tidy.
 // UNSUPPORTED{BLOCKLIT}: gcc
 
+// Clang 17 has false positives.
+// UNSUPPORTED{BLOCKLIT}: clang-17
+
 {lit_header_restrictions.get(header, '')}
 
 // TODO: run clang-tidy with modules enabled once they are supported
diff --git a/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt b/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
index 28eed61445833..aa5c2a0a1d900 100644
--- a/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
+++ b/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
@@ -64,6 +64,26 @@ if(NOT HAS_CLANG_TIDY_HEADERS)
                  "clang-tidy headers are not present.")
   return()
 endif()
+
+# The clangTidy plugin uses C++20, when using stdlibc++ verify the version is correct.
+# Note it has not been tested whether version 11 works.
+file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/test.cpp" "
+#include <version>
+#if defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE < 12
+  # error The stdlibc++ version is too old.
+#endif
+int main(){}
+")
+try_compile(HAS_NEWER_STANDARD_LIBRARY
+  "${CMAKE_CURRENT_BINARY_DIR}"
+  "${CMAKE_CURRENT_BINARY_DIR}/test.cpp"
+   LINK_LIBRARIES clangTidy)
+
+if(NOT HAS_NEWER_STANDARD_LIBRARY)
+  message(STATUS "Clang-tidy tests are disabled due to using "
+                 "stdlibc++ older than version 12")
+  return()
+endif()
 message(STATUS "Clang-tidy tests are enabled.")
 
 set(SOURCES
@@ -88,5 +108,3 @@ set_target_properties(cxx-tidy PROPERTIES
 
 set_target_properties(cxx-tidy PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
 set(CMAKE_SHARED_MODULE_SUFFIX_CXX .plugin) # Use a portable suffix to simplify how we can find it from Lit
-
-list(APPEND LIBCXX_TEST_DEPS cxx-tidy)

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w/ some comments.

libcxx/test/tools/clang_tidy_checks/CMakeLists.txt Outdated Show resolved Hide resolved
libcxx/test/tools/clang_tidy_checks/CMakeLists.txt Outdated Show resolved Hide resolved
@mordante mordante force-pushed the review/reenables_clang_tidy_in_ci branch 2 times, most recently from f0a539f to ab68fe4 Compare May 7, 2024 19:02
This is currently a test patch; locally clang-tidy seems to be used in
the Docker image.
@mordante mordante force-pushed the review/reenables_clang_tidy_in_ci branch from ab68fe4 to 0bac7e0 Compare May 8, 2024 15:24
@mordante mordante merged commit 79921fb into llvm:main May 8, 2024
50 of 51 checks passed
@mordante mordante deleted the review/reenables_clang_tidy_in_ci branch May 8, 2024 18:18
mordante added a commit to mordante/llvm-project that referenced this pull request Jun 15, 2024
There have been some reports that always enabling clang-tidy (added in
llvm#90077) has some false positives in the detection. It would be good to fix
these. This adds an escape hatch to disable the tests, which avoids
blocking users with this issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants