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] Fix msvc_is_lock_free_macro_value() #105876

Merged
merged 2 commits into from
Aug 24, 2024

Conversation

StephanTLavavej
Copy link
Member

Followup to #99570.

  • TEST_COMPILER_MSVC must be tested for definedness, as it is everywhere else.
  • Fix bogus return type: msvc_is_lock_free_macro_value() returns 2 or 0, so it needs to return int.
    • Fixes: llvm-project\libcxx\test\support\atomic_helpers.h(41): warning C4305: 'return': truncation from 'int' to 'bool'
  • Clarity improvement: also add parens when mixing bitwise with arithmetic operators.

Followup to LLVM-99570.

Fixes:
llvm-project\libcxx\test\support\atomic_helpers.h(33): fatal error C1017: invalid integer constant expression
Fixes:
llvm-project\libcxx\test\support\atomic_helpers.h(41): warning C4305: 'return': truncation from 'int' to 'bool'

For clarity, also add parens when mixing bitwise with arithmetic operators.
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner August 23, 2024 19:22
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 23, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 23, 2024

@llvm/pr-subscribers-libcxx

Author: Stephan T. Lavavej (StephanTLavavej)

Changes

Followup to #99570.

  • TEST_COMPILER_MSVC must be tested for definedness, as it is everywhere else.
  • Fix bogus return type: msvc_is_lock_free_macro_value() returns 2 or 0, so it needs to return int.
    • Fixes: llvm-project\libcxx\test\support\atomic_helpers.h(41): warning C4305: 'return': truncation from 'int' to 'bool'
  • Clarity improvement: also add parens when mixing bitwise with arithmetic operators.

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

1 Files Affected:

  • (modified) libcxx/test/support/atomic_helpers.h (+3-3)
diff --git a/libcxx/test/support/atomic_helpers.h b/libcxx/test/support/atomic_helpers.h
index d2f2b751cb47de..2b3a3caa06a589 100644
--- a/libcxx/test/support/atomic_helpers.h
+++ b/libcxx/test/support/atomic_helpers.h
@@ -30,15 +30,15 @@
 #  define TEST_ATOMIC_LONG_LOCK_FREE __GCC_ATOMIC_LONG_LOCK_FREE
 #  define TEST_ATOMIC_LLONG_LOCK_FREE __GCC_ATOMIC_LLONG_LOCK_FREE
 #  define TEST_ATOMIC_POINTER_LOCK_FREE __GCC_ATOMIC_POINTER_LOCK_FREE
-#elif TEST_COMPILER_MSVC
+#elif defined(TEST_COMPILER_MSVC)
 // This is lifted from STL/stl/inc/atomic on github for the purposes of
 // keeping the tests compiling for MSVC's STL. It's not a perfect solution
 // but at least the tests will keep running.
 //
 // Note MSVC's STL never produces a type that is sometimes lock free, but not always lock free.
 template <class T, size_t Size = sizeof(T)>
-constexpr bool msvc_is_lock_free_macro_value() {
-  return (Size <= 8 && (Size & Size - 1) == 0) ? 2 : 0;
+constexpr int msvc_is_lock_free_macro_value() {
+  return (Size <= 8 && (Size & (Size - 1)) == 0) ? 2 : 0;
 }
 #  define TEST_ATOMIC_CHAR_LOCK_FREE ::msvc_is_lock_free_macro_value<char>()
 #  define TEST_ATOMIC_SHORT_LOCK_FREE ::msvc_is_lock_free_macro_value<short>()

@StephanTLavavej
Copy link
Member Author

Thanks! I'll go ahead and merge this as all of the GH checks have passed, and all of the BuildKite checks except that 4 Apple configurations have been waiting for agents for 21 hours - my changes are very clearly limited to MSVC so there should be no concern there.

@StephanTLavavej StephanTLavavej merged commit 886b761 into llvm:main Aug 24, 2024
56 of 58 checks passed
@StephanTLavavej StephanTLavavej deleted the test-compiler-msvc branch August 24, 2024 16:55
dmpolukhin pushed a commit to dmpolukhin/llvm-project that referenced this pull request Sep 2, 2024
Followup to llvm#99570.

* `TEST_COMPILER_MSVC` must be tested for `defined`ness, as it is
everywhere else.
+ Definition:
https://github.com/llvm/llvm-project/blob/52a7116f5c6ada234f47f7794aaf501a3692b997/libcxx/test/support/test_macros.h#L71-L72
+ Example usage:
https://github.com/llvm/llvm-project/blob/52a7116f5c6ada234f47f7794aaf501a3692b997/libcxx/test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp#L248
+ Fixes: `llvm-project\libcxx\test\support\atomic_helpers.h(33): fatal
error C1017: invalid integer constant expression`
* Fix bogus return type: `msvc_is_lock_free_macro_value()` returns `2`
or `0`, so it needs to return `int`.
+ Fixes: `llvm-project\libcxx\test\support\atomic_helpers.h(41): warning
C4305: 'return': truncation from 'int' to 'bool'`
* Clarity improvement: also add parens when mixing bitwise with
arithmetic operators.
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