Skip to content

Conversation

@StephanTLavavej
Copy link
Member

Found while running libc++'s test suite with MSVC's STL, where we use both MSVC's compiler and Clang/LLVM.

MSVC really enjoys emitting "warning C4305: 'initializing': truncation from 'double' to 'float'". It might look repetitive, but T(value) avoids this warning. (According to my understanding, the compiler looks at the immediate context, and if it's a functional-style cast, it's satisfied that the truncation was intentional. Not even the direct-initialization context of T unexpected(-9999.99) is enough to activate that "ok, the programmer knows what they're getting" codepath.)

I usually prefer to use static_cast instead of functional-style casts, but I chose to remain consistent with the surrounding code.

By the way, you might notice that 1.5 doesn't need these changes. This is because it's exactly representable as both a double and a float. Values like 3.125 instead of 3.1 would similarly avoid truncation warnings without the need for casts, but I didn't want to intrusively change the test code.

It might look repetitive, but T(value) is what avoids this warning.

I usually prefer to use static_cast instead of functional-style casts, but I chose to remain consistent with the surrounding code.
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner December 2, 2023 06:58
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 2, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 2, 2023

@llvm/pr-subscribers-libcxx

Author: Stephan T. Lavavej (StephanTLavavej)

Changes

Found while running libc++'s test suite with MSVC's STL, where we use both MSVC's compiler and Clang/LLVM.

MSVC really enjoys emitting "warning C4305: 'initializing': truncation from 'double' to 'float'". It might look repetitive, but T(value) avoids this warning. (According to my understanding, the compiler looks at the immediate context, and if it's a functional-style cast, it's satisfied that the truncation was intentional. Not even the direct-initialization context of T unexpected(-9999.99) is enough to activate that "ok, the programmer knows what they're getting" codepath.)

I usually prefer to use static_cast instead of functional-style casts, but I chose to remain consistent with the surrounding code.

By the way, you might notice that 1.5 doesn't need these changes. This is because it's exactly representable as both a double and a float. Values like 3.125 instead of 3.1 would similarly avoid truncation warnings without the need for casts, but I didn't want to intrusively change the test code.


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

15 Files Affected:

  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/assign.pass.cpp (+1-1)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/compare_exchange_strong.pass.cpp (+7-7)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/compare_exchange_weak.pass.cpp (+7-7)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/exchange.pass.cpp (+1-1)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/fetch_add.pass.cpp (+2-2)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/fetch_sub.pass.cpp (+2-2)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/load.pass.cpp (+1-1)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/notify_all.pass.cpp (+1-1)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/notify_one.pass.cpp (+1-1)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.float.pass.cpp (+1-1)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.minus_equals.pass.cpp (+2-2)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.plus_equals.pass.cpp (+2-2)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/store.pass.cpp (+1-1)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/test_helper.h (+1-1)
  • (modified) libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/wait.pass.cpp (+1-1)
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/assign.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/assign.pass.cpp
index 3daf3aba71fc9f2..66f6c91b802cb0c 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/assign.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/assign.pass.cpp
@@ -31,7 +31,7 @@ void test_impl() {
 
   // assignment
   {
-    MaybeVolatile<std::atomic<T>> a(3.1);
+    MaybeVolatile<std::atomic<T>> a(T(3.1));
     std::same_as<T> decltype(auto) r = (a = T(1.2));
     assert(a.load() == T(1.2));
     assert(r == T(1.2));
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/compare_exchange_strong.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/compare_exchange_strong.pass.cpp
index b010e7c7a4d79d9..711b49ddc233028 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/compare_exchange_strong.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/compare_exchange_strong.pass.cpp
@@ -44,8 +44,8 @@ void testBasic(MemoryOrder... memory_order) {
   // compare pass
   {
     MaybeVolatile<std::atomic<T>> a(T(1.2));
-    T expected(1.2);
-    const T desired(2.3);
+    T expected(T(1.2));
+    const T desired(T(2.3));
     std::same_as<bool> decltype(auto) r = a.compare_exchange_strong(expected, desired, memory_order...);
 
     assert(r);
@@ -57,7 +57,7 @@ void testBasic(MemoryOrder... memory_order) {
   {
     MaybeVolatile<std::atomic<T>> a(T(1.2));
     T expected(1.5);
-    const T desired(2.3);
+    const T desired(T(2.3));
     std::same_as<bool> decltype(auto) r = a.compare_exchange_strong(expected, desired, memory_order...);
 
     assert(!r);
@@ -168,7 +168,7 @@ void test_impl() {
     auto store = [](MaybeVolatile<std::atomic<T>>& x, T, T new_val) { x.store(new_val, std::memory_order::release); };
     auto load  = [](MaybeVolatile<std::atomic<T>>& x) {
       auto result = x.load(std::memory_order::relaxed);
-      T unexpected(-9999.99);
+      T unexpected(T(-9999.99));
       bool r = x.compare_exchange_strong(unexpected, unexpected, std::memory_order_relaxed, std::memory_order_acquire);
       assert(!r);
       return result;
@@ -177,7 +177,7 @@ void test_impl() {
 
     auto load_one_arg = [](MaybeVolatile<std::atomic<T>>& x) {
       auto result = x.load(std::memory_order::relaxed);
-      T unexpected(-9999.99);
+      T unexpected(T(-9999.99));
       bool r = x.compare_exchange_strong(unexpected, unexpected, std::memory_order_acquire);
       assert(!r);
       return result;
@@ -187,7 +187,7 @@ void test_impl() {
     // acq_rel replaced by acquire
     auto load_one_arg_acq_rel = [](MaybeVolatile<std::atomic<T>>& x) {
       auto result = x.load(std::memory_order::relaxed);
-      T unexpected(-9999.99);
+      T unexpected(T(-9999.99));
       bool r = x.compare_exchange_strong(unexpected, unexpected, std::memory_order_acq_rel);
       assert(!r);
       return result;
@@ -200,7 +200,7 @@ void test_impl() {
     auto store = [](MaybeVolatile<std::atomic<T>>& x, T, T new_val) { x.store(new_val, std::memory_order::seq_cst); };
     auto load  = [](MaybeVolatile<std::atomic<T>>& x) {
       auto result = x.load(std::memory_order::relaxed);
-      T unexpected(-9999.99);
+      T unexpected(T(-9999.99));
       bool r = x.compare_exchange_strong(unexpected, unexpected, std::memory_order_relaxed, std::memory_order::seq_cst);
       assert(!r);
       return result;
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/compare_exchange_weak.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/compare_exchange_weak.pass.cpp
index 27853ef08c7482c..ee1a00c284276c8 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/compare_exchange_weak.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/compare_exchange_weak.pass.cpp
@@ -44,8 +44,8 @@ void testBasic(MemoryOrder... memory_order) {
   // compare pass
   {
     MaybeVolatile<std::atomic<T>> a(T(1.2));
-    T expected(1.2);
-    const T desired(2.3);
+    T expected(T(1.2));
+    const T desired(T(2.3));
     std::same_as<bool> decltype(auto) r = a.compare_exchange_weak(expected, desired, memory_order...);
 
     // could be false spuriously
@@ -61,7 +61,7 @@ void testBasic(MemoryOrder... memory_order) {
   {
     MaybeVolatile<std::atomic<T>> a(T(1.2));
     T expected(1.5);
-    const T desired(2.3);
+    const T desired(T(2.3));
     std::same_as<bool> decltype(auto) r = a.compare_exchange_weak(expected, desired, memory_order...);
 
     assert(!r);
@@ -183,7 +183,7 @@ void test_impl() {
     auto store = [](MaybeVolatile<std::atomic<T>>& x, T, T new_val) { x.store(new_val, std::memory_order::release); };
     auto load  = [](MaybeVolatile<std::atomic<T>>& x) {
       auto result = x.load(std::memory_order::relaxed);
-      T unexpected(-9999.99);
+      T unexpected(T(-9999.99));
       bool r = x.compare_exchange_weak(unexpected, unexpected, std::memory_order_relaxed, std::memory_order_acquire);
       assert(!r);
       return result;
@@ -192,7 +192,7 @@ void test_impl() {
 
     auto load_one_arg = [](MaybeVolatile<std::atomic<T>>& x) {
       auto result = x.load(std::memory_order::relaxed);
-      T unexpected(-9999.99);
+      T unexpected(T(-9999.99));
       bool r = x.compare_exchange_weak(unexpected, unexpected, std::memory_order_acquire);
       assert(!r);
       return result;
@@ -202,7 +202,7 @@ void test_impl() {
     // acq_rel replaced by acquire
     auto load_one_arg_acq_rel = [](MaybeVolatile<std::atomic<T>>& x) {
       auto result = x.load(std::memory_order::relaxed);
-      T unexpected(-9999.99);
+      T unexpected(T(-9999.99));
       bool r = x.compare_exchange_weak(unexpected, unexpected, std::memory_order_acq_rel);
       assert(!r);
       return result;
@@ -215,7 +215,7 @@ void test_impl() {
     auto store = [](MaybeVolatile<std::atomic<T>>& x, T, T new_val) { x.store(new_val, std::memory_order::seq_cst); };
     auto load  = [](MaybeVolatile<std::atomic<T>>& x) {
       auto result = x.load(std::memory_order::relaxed);
-      T unexpected(-9999.99);
+      T unexpected(T(-9999.99));
       bool r = x.compare_exchange_weak(unexpected, unexpected, std::memory_order_relaxed, std::memory_order::seq_cst);
       assert(!r);
       return result;
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/exchange.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/exchange.pass.cpp
index 9c67d7e671cfa71..c0d1eb686db7a08 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/exchange.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/exchange.pass.cpp
@@ -34,7 +34,7 @@ void test_impl() {
 
   // exchange
   {
-    MaybeVolatile<std::atomic<T>> a(3.1);
+    MaybeVolatile<std::atomic<T>> a(T(3.1));
     std::same_as<T> decltype(auto) r = a.exchange(T(1.2), std::memory_order::relaxed);
     assert(a.load() == T(1.2));
     assert(r == T(3.1));
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/fetch_add.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/fetch_add.pass.cpp
index 664185fc243f9a2..c31243053e69fba 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/fetch_add.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/fetch_add.pass.cpp
@@ -42,7 +42,7 @@ void test_impl() {
 
   // fetch_add
   {
-    MaybeVolatile<std::atomic<T>> a(3.1);
+    MaybeVolatile<std::atomic<T>> a(T(3.1));
     std::same_as<T> decltype(auto) r = a.fetch_add(T(1.2), std::memory_order::relaxed);
     assert(r == T(3.1));
     assert(a.load() == T(3.1) + T(1.2));
@@ -78,7 +78,7 @@ void test_impl() {
       return res;
     };
 
-    assert(at.load() == times(1.234, number_of_threads * loop));
+    assert(at.load() == times(T(1.234), number_of_threads * loop));
   }
 #endif
 
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/fetch_sub.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/fetch_sub.pass.cpp
index c4e33538de74178..d26d21dec1b3346 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/fetch_sub.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/fetch_sub.pass.cpp
@@ -43,7 +43,7 @@ void test_impl() {
 
   // fetch_sub
   {
-    MaybeVolatile<std::atomic<T>> a(3.1);
+    MaybeVolatile<std::atomic<T>> a(T(3.1));
     std::same_as<T> decltype(auto) r = a.fetch_sub(T(1.2), std::memory_order::relaxed);
     assert(r == T(3.1));
     assert(a.load() == T(3.1) - T(1.2));
@@ -79,7 +79,7 @@ void test_impl() {
       return res;
     };
 
-    assert(at.load() == accu_neg(1.234, number_of_threads * loop));
+    assert(at.load() == accu_neg(T(1.234), number_of_threads * loop));
   }
 #endif
 
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/load.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/load.pass.cpp
index b495d04989e2007..b567af457dcaecd 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/load.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/load.pass.cpp
@@ -41,7 +41,7 @@ void test_impl() {
 
   // load
   {
-    MaybeVolatile<std::atomic<T>> a(3.1);
+    MaybeVolatile<std::atomic<T>> a(T(3.1));
     a.store(T(1.2));
     std::same_as<T> decltype(auto) r = a.load(std::memory_order::relaxed);
     assert(r == T(1.2));
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/notify_all.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/notify_all.pass.cpp
index eca7a19e5c57da0..b50c0ad9ee92efb 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/notify_all.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/notify_all.pass.cpp
@@ -37,7 +37,7 @@ void test_impl() {
   // should x87 80bit long double work at all?
   if constexpr (!std::same_as<T, long double>) {
     for (auto i = 0; i < 100; ++i) {
-      const T old = 3.1;
+      const T old = T(3.1);
       MaybeVolatile<std::atomic<T>> a(old);
 
       bool done                      = false;
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/notify_one.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/notify_one.pass.cpp
index 183cf18b820e746..aee4af186b9ffe3 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/notify_one.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/notify_one.pass.cpp
@@ -37,7 +37,7 @@ void test_impl() {
   // should x87 80bit long double work at all?
   if constexpr (!std::same_as<T, long double>) {
     for (auto i = 0; i < 100; ++i) {
-      const T old = 3.1;
+      const T old = T(3.1);
       MaybeVolatile<std::atomic<T>> a(old);
 
       std::atomic_bool started = false;
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.float.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.float.pass.cpp
index 58c8da6c1042b26..5d957ca68404a23 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.float.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.float.pass.cpp
@@ -28,7 +28,7 @@ void test_impl() {
 
   // operator float
   {
-    MaybeVolatile<std::atomic<T>> a(3.1);
+    MaybeVolatile<std::atomic<T>> a(T(3.1));
     T r = a;
     assert(r == T(3.1));
   }
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.minus_equals.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.minus_equals.pass.cpp
index b1ef276d870cf50..269eb819524f9d4 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.minus_equals.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.minus_equals.pass.cpp
@@ -39,7 +39,7 @@ void test_impl() {
 
   // -=
   {
-    MaybeVolatile<std::atomic<T>> a(3.1);
+    MaybeVolatile<std::atomic<T>> a(T(3.1));
     std::same_as<T> decltype(auto) r = a -= T(1.2);
     assert(r == T(3.1) - T(1.2));
     assert(a.load() == T(3.1) - T(1.2));
@@ -75,7 +75,7 @@ void test_impl() {
       return res;
     };
 
-    assert(at.load() == accu_neg(1.234, number_of_threads * loop));
+    assert(at.load() == accu_neg(T(1.234), number_of_threads * loop));
   }
 #endif
 
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.plus_equals.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.plus_equals.pass.cpp
index 0b1781bf8e571b9..9a2298ea46d660b 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.plus_equals.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/operator.plus_equals.pass.cpp
@@ -39,7 +39,7 @@ void test_impl() {
 
   // +=
   {
-    MaybeVolatile<std::atomic<T>> a(3.1);
+    MaybeVolatile<std::atomic<T>> a(T(3.1));
     std::same_as<T> decltype(auto) r = a += T(1.2);
     assert(r == T(3.1) + T(1.2));
     assert(a.load() == T(3.1) + T(1.2));
@@ -75,7 +75,7 @@ void test_impl() {
       return res;
     };
 
-    assert(at.load() == times(1.234, number_of_threads * loop));
+    assert(at.load() == times(T(1.234), number_of_threads * loop));
   }
 
   // memory_order::seq_cst
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/store.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/store.pass.cpp
index 81d17cd32a6c502..b0888aac67dbbf5 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/store.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/store.pass.cpp
@@ -41,7 +41,7 @@ void test_impl() {
 
   // store
   {
-    MaybeVolatile<std::atomic<T>> a(3.1);
+    MaybeVolatile<std::atomic<T>> a(T(3.1));
     a.store(T(1.2), std::memory_order::relaxed);
     assert(a.load() == T(1.2));
   }
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/test_helper.h b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/test_helper.h
index b831b774fbd3073..d0da1f20f550507 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/test_helper.h
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/test_helper.h
@@ -23,7 +23,7 @@
 
 template <class T>
 bool approximately_equals(T x, T y) {
-  T epsilon = 0.001;
+  T epsilon = T(0.001);
   return std::abs(x - y) < epsilon;
 }
 
diff --git a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/wait.pass.cpp b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/wait.pass.cpp
index d95801e25d35e03..8e003a7e365da1a 100644
--- a/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/wait.pass.cpp
+++ b/libcxx/test/std/atomics/atomics.types.generic/atomics.types.float/wait.pass.cpp
@@ -51,7 +51,7 @@ void test_impl() {
   // should x87 80bit long double work at all?
   if constexpr (!std::same_as<T, long double>) {
     for (auto i = 0; i < 100; ++i) {
-      const T old = 3.1;
+      const T old = T(3.1);
       MaybeVolatile<std::atomic<T>> a(old);
 
       std::atomic_bool started = false;

@tschuett
Copy link

tschuett commented Dec 2, 2023

https://llvm.org/docs/CodingStandards.html#prefer-c-style-casts
The cast style is documented.

@StephanTLavavej
Copy link
Member Author

Checks are green modulo spurious git failures, merging...

@StephanTLavavej StephanTLavavej merged commit 091a9f4 into llvm:main Dec 3, 2023
@StephanTLavavej StephanTLavavej deleted the stl-13-double-trouble branch December 3, 2023 01:40
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