Skip to content

Conversation

@mordante
Copy link
Member

@mordante mordante commented Aug 6, 2024

The tools IWYU/clangd can suggest to remove unused headers. This suggests to remove test_macros.h. Removing this header can break tests.

For example tests use
#ifndef TEST_HAS_NO_WIDE_CHARACTERS
test<wchar_t>();
#endif

Since developers typically have wide characters support, the header is not used. However, removing the header would break configurations that have wide characters disabled.

The tools IWYU/clangd can suggest to remove unused headers. This
suggests to remove test_macros.h. Removing this header can break tests.

For example tests use
   #ifndef TEST_HAS_NO_WIDE_CHARACTERS
     test<wchar_t>();
   #endif

Since typically developers have wide characters the header is not used.
The removal of the header breaks configurations that have wide
characters disabled.
@mordante mordante requested a review from a team as a code owner August 6, 2024 17:59
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 6, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 6, 2024

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

The tools IWYU/clangd can suggest to remove unused headers. This suggests to remove test_macros.h. Removing this header can break tests.

For example tests use
#ifndef TEST_HAS_NO_WIDE_CHARACTERS
test<wchar_t>();
#endif

Since typically developers have wide characters the header is not used. The removal of the header breaks configurations that have wide characters disabled.


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

1 Files Affected:

  • (modified) libcxx/test/support/test_macros.h (+6)
diff --git a/libcxx/test/support/test_macros.h b/libcxx/test/support/test_macros.h
index 15fc5b69b52076..9dab5750a5f917 100644
--- a/libcxx/test/support/test_macros.h
+++ b/libcxx/test/support/test_macros.h
@@ -7,6 +7,12 @@
 //
 //===----------------------------------------------------------------------===//
 
+// This header contains several configuration defines.
+// Removing the header would #ifndef TEST_FOO to unconditionally pass when
+// it's optionally defined in this header.
+//
+// IWYU pragma: always_keep
+
 #ifndef SUPPORT_TEST_MACROS_HPP
 #define SUPPORT_TEST_MACROS_HPP
 

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.

My preference would be to instead use 0-1 for TEST_HAS_XXX macros, since that also solves the existing problem that our test suite is not safe w.r.t. -Wundef. And it would solve the clang-tidy issues since we'd actually use the macro from the header.

@mordante
Copy link
Member Author

mordante commented Feb 9, 2025

As discussed at a libc++ monthly we want to move in the direction as suggested by @ldionne above.

@mordante mordante closed this Feb 9, 2025
@mordante mordante deleted the review/use_iwyu_pragmas branch February 9, 2025 13:00
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.

3 participants