Skip to content

[libc++][test] Add missing <concepts> to is_always_lock_free test#105966

Merged
ldionne merged 2 commits intollvm:mainfrom
StephanTLavavej:fix-atomic-test
Aug 26, 2024
Merged

[libc++][test] Add missing <concepts> to is_always_lock_free test#105966
ldionne merged 2 commits intollvm:mainfrom
StephanTLavavej:fix-atomic-test

Conversation

@StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Aug 25, 2024

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

StephanTLavavej commented Aug 25, 2024

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?

@StephanTLavavej StephanTLavavej mentioned this pull request Aug 25, 2024
Merged
@CaseyCarter
Copy link
Member

CaseyCarter commented Aug 25, 2024

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

@h-vetinari h-vetinari mentioned this pull request Aug 26, 2024
Merged
@ldionne ldionne merged commit bc695f5 into llvm:main Aug 26, 2024
@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
@StephanTLavavej StephanTLavavej mentioned this pull request Oct 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ldionne ldionne ldionne approved these changes

Assignees

No one assigned

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@StephanTLavavej @llvmbot @CaseyCarter @ldionne