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++][test] Add missing <concepts> to is_always_lock_free test #105966

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

StephanTLavavej
Copy link
Member

There were a couple of problems with this test added by #99570:

  • It was named is_always_lock_free.cpp instead of is_always_lock_free.pass.cpp.
    • Noticed because the MSVC STL test harness looks for MEOW.pass.cpp specifically.
  • It was using std::same_as without including <concepts>.
    • Noticed because MSVC's STL doesn't drag this in via other headers.

@StephanTLavavej StephanTLavavej requested a review from a team as a code owner August 25, 2024 02:06
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 25, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 25, 2024

@llvm/pr-subscribers-libcxx

Author: Stephan T. Lavavej (StephanTLavavej)

Changes

There were a couple of problems with this test added by #99570:

  • It was named is_always_lock_free.cpp instead of is_always_lock_free.pass.cpp.
    • Noticed because the MSVC STL test harness looks for MEOW.pass.cpp specifically.
  • It was using std::same_as without including &lt;concepts&gt;.
    • Noticed because MSVC's STL doesn't drag this in via other headers.

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

1 Files Affected:

  • (renamed) libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.pass.cpp (+1)
diff --git a/libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.cpp b/libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.pass.cpp
similarity index 99%
rename from libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.cpp
rename to libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.pass.cpp
index 2dc7f5c7654193..db17221e515d3a 100644
--- a/libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.cpp
+++ b/libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.pass.cpp
@@ -17,6 +17,7 @@
 
 #include <atomic>
 #include <cassert>
+#include <concepts>
 #include <cstddef>
 
 #include "test_macros.h"

@StephanTLavavej
Copy link
Member Author

Now that the test is correctly named, it's also being picked up by libc++'s test harness and experiencing other problems:

# | /home/runner/_work/llvm-project/llvm-project/libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.pass.cpp:102:3: error: AVX vector argument of type '__attribute__((__vector_size__(16 * sizeof(int)))) int' (vector of 16 'int' values) without 'avx512f' enabled changes the ABI [-Werror,-Wpsabi]
# |   102 |   CHECK_ALWAYS_LOCK_FREE(int __attribute__((vector_size(16 * sizeof(int)))));
# |       |   ^
# | /home/runner/_work/llvm-project/llvm-project/libcxx/test/std/atomics/atomics.lockfree/is_always_lock_free.pass.cpp:58:23: note: expanded from macro 'CHECK_ALWAYS_LOCK_FREE'
# |    58 |     std::atomic<type> a(obj);                                                                                          \
# |       |                       ^

I'm not tooled up to fix these (it's passing for MSVC's STL), can a real libc++ dev help?

@CaseyCarter
Copy link
Member

Poking @ldionne, who added this test case in #99570.

@ldionne ldionne merged commit bc695f5 into llvm:main Aug 26, 2024
13 of 18 checks passed
@ldionne
Copy link
Member

ldionne commented Aug 26, 2024

Jeez. I merged without noticing that this PR was also renaming the test (triggering other failures).

@ldionne
Copy link
Member

ldionne commented Aug 26, 2024

I undid the renaming for now and took only the addition of <concepts>: 0f58ab8

I am tackling the renaming (and the additional required fixes) in #106077

@ldionne ldionne changed the title [libc++][test] Fix is_always_lock_free test [libc++][test] Add missing <concepts> to is_always_lock_free test Aug 26, 2024
@StephanTLavavej StephanTLavavej deleted the fix-atomic-test branch August 26, 2024 16:41
5c4lar pushed a commit to 5c4lar/llvm-project that referenced this pull request Aug 29, 2024
…vm#105966)

That test was using std::same_as without including <concepts>.
dmpolukhin pushed a commit to dmpolukhin/llvm-project that referenced this pull request Sep 2, 2024
…vm#105966)

That test was using std::same_as without including <concepts>.
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.

4 participants