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] Make macro detection more friendly to MSVC #113633

Merged
merged 1 commit into from
Oct 26, 2024

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Oct 25, 2024

MSVC STL's test suite is a bit nervous about replacing non-macro-defined identifiers with 0 (see also https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4668?view=msvc-170).

On MSVC (and MS-compatible mode of other compilers), long double has the same format (IEEE-754 binary64) as double, so it should be OK to define TEST_LONG_DOUBLE_IS_DOUBLE when _MSC_VER is defined. Such detection should be performed first.

MSVC's test suite is a bit nervous about replacing non-macro-defined identifiers with `0` (see also https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4668?view=msvc-170).

On MSVC (and MS-compatible mode of other compilers), `long double` has the same format (IEEE-754 binary64) as  `double`, so it should be OK to define `TEST_LONG_DOUBLE_IS_DOUBLE` when `_MSC_VER` is defined. Such detection should be performed first.
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner October 25, 2024 02:19
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 25, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 25, 2024

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

MSVC STL's test suite is a bit nervous about replacing non-macro-defined identifiers with 0 (see also https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4668?view=msvc-170).

On MSVC (and MS-compatible mode of other compilers), long double has the same format (IEEE-754 binary64) as double, so it should be OK to define TEST_LONG_DOUBLE_IS_DOUBLE when _MSC_VER is defined. Such detection should be performed first.


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

1 Files Affected:

  • (modified) libcxx/test/support/test_macros.h (+1-1)
diff --git a/libcxx/test/support/test_macros.h b/libcxx/test/support/test_macros.h
index 5ef14e54dae237..1b6473b623c53b 100644
--- a/libcxx/test/support/test_macros.h
+++ b/libcxx/test/support/test_macros.h
@@ -511,7 +511,7 @@ inline Tp const& DoNotOptimize(Tp const& value) {
 #  define TEST_CONSTEXPR_OPERATOR_NEW
 #endif
 
-#if __SIZEOF_LONG_DOUBLE__ == __SIZEOF_DOUBLE__
+#if defined(_MSC_VER) || __SIZEOF_LONG_DOUBLE__ == __SIZEOF_DOUBLE__
 #  define TEST_LONG_DOUBLE_IS_DOUBLE
 #endif
 

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 25, 2024

This looks good to me.

Edit: EDG on Windows doesn't understand these macros, so _MSC_VER is proper to use here IMO; TEST_COMPILER_MSVC would miss EDG.

C:\Temp>type meow.cpp
#include <print>
using namespace std;

int main() {
    println("__SIZEOF_LONG_DOUBLE__: {}", __SIZEOF_LONG_DOUBLE__);
    println("__SIZEOF_DOUBLE__: {}", __SIZEOF_DOUBLE__);
}
C:\Temp>cl /EHsc /nologo /W4 /std:c++latest meow.cpp && meow
meow.cpp
meow.cpp(5): error C2065: '__SIZEOF_LONG_DOUBLE__': undeclared identifier
meow.cpp(6): error C2065: '__SIZEOF_DOUBLE__': undeclared identifier

C:\Temp>clang-cl /EHsc /nologo /W4 /std:c++latest meow.cpp && meow
__SIZEOF_LONG_DOUBLE__: 8
__SIZEOF_DOUBLE__: 8

C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /c /BE meow.cpp
meow.cpp
"meow.cpp", line 5: error: identifier "__SIZEOF_LONG_DOUBLE__" is undefined
      println("__SIZEOF_LONG_DOUBLE__: {}", __SIZEOF_LONG_DOUBLE__);
                                            ^

"meow.cpp", line 5: error: no instance of overloaded function "println" matches
          the argument list
            argument types are: (const char [27], <error type>)
      println("__SIZEOF_LONG_DOUBLE__: {}", __SIZEOF_LONG_DOUBLE__);
      ^

"meow.cpp", line 6: error: identifier "__SIZEOF_DOUBLE__" is undefined
      println("__SIZEOF_DOUBLE__: {}", __SIZEOF_DOUBLE__);
                                       ^

"meow.cpp", line 6: error: no instance of overloaded function "println" matches
          the argument list
            argument types are: (const char [22], <error type>)
      println("__SIZEOF_DOUBLE__: {}", __SIZEOF_DOUBLE__);
      ^

@frederick-vs-ja frederick-vs-ja merged commit 3fc0d94 into llvm:main Oct 26, 2024
65 checks passed
@frederick-vs-ja frederick-vs-ja deleted the msvc-long-double branch October 26, 2024 05:47
winner245 pushed a commit to winner245/llvm-project that referenced this pull request Oct 26, 2024
MSVC STL's test suite is a bit nervous about replacing non-macro-defined
identifiers with `0` (see also
https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4668?view=msvc-170).

On MSVC (and MS-compatible mode of other compilers), `long double` has
the same format (IEEE-754 binary64) as `double`, so it should be OK to
define `TEST_LONG_DOUBLE_IS_DOUBLE` when `_MSC_VER` is defined. Such
detection should be performed first.
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
MSVC STL's test suite is a bit nervous about replacing non-macro-defined
identifiers with `0` (see also
https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4668?view=msvc-170).

On MSVC (and MS-compatible mode of other compilers), `long double` has
the same format (IEEE-754 binary64) as `double`, so it should be OK to
define `TEST_LONG_DOUBLE_IS_DOUBLE` when `_MSC_VER` is defined. Such
detection should be performed first.
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. test-suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants