[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal updated this revision to Diff 98219.
BillyONeal marked 2 inline comments as done.
BillyONeal edited the summary of this revision.
BillyONeal added a comment.

Do Eric's suggestions.


https://reviews.llvm.org/D32309

Files:
  test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
  test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp

Index: test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
===
--- test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
+++ test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
@@ -11,12 +11,14 @@
 // 
 
 // template
-// constexpr common_type_t<_M,_N> gcd(_M __m, _N __n)
+// constexpr common_type_t<_M,_N> lcm(_M __m, _N __n)
 
 #include 
 #include 
+#include 
+#include 
 #include 
-#include 
+#include 
 
 constexpr struct {
   int x;
@@ -34,21 +36,24 @@
 };
 
 template 
-constexpr bool test0(Input1 in1, Input2 in2, Output out)
+constexpr bool test0(int in1, int in2, int out)
 {
-static_assert((std::is_same::value), "" );
-static_assert((std::is_same::value), "" );
-return out == std::lcm(in1, in2) ? true : (std::abort(), false);
+auto value1 = static_cast(in1);
+auto value2 = static_cast(in2);
+static_assert(std::is_same_v, "");
+static_assert(std::is_same_v, "");
+assert(static_cast(out) == std::lcm(value1, value2));
+return true;
 }
 
 
 template 
 constexpr bool do_test(int = 0)
 {
-using S1 = typename std::make_signed::type;
-using S2 = typename std::make_signed::type;
-using U1 = typename std::make_unsigned::type;
-using U2 = typename std::make_unsigned::type;
+using S1 = std::make_signed_t;
+using S2 = std::make_signed_t;
+using U1 = std::make_unsigned_t;
+using U2 = std::make_unsigned_t;
 bool accumulate = true;
 for (auto TC : Cases) {
 { // Test with two signed types
@@ -101,15 +106,15 @@
 assert(do_test(non_cce));
 assert(do_test(non_cce));
 
-static_assert(do_test< int8_t>(), "");
-static_assert(do_test(), "");
-static_assert(do_test(), "");
-static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
 
-assert(do_test< int8_t>(non_cce));
-assert(do_test(non_cce));
-assert(do_test(non_cce));
-assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
 
 static_assert(do_test(), "");
 static_assert(do_test(), "");
@@ -131,9 +136,9 @@
 
 //  LWG#2837
 {
-auto res1 = std::lcm((int64_t)1234, (int32_t)-2147483648);
-(void) std::lcm(INT_MIN, 2);	// this used to trigger UBSAN
-static_assert( std::is_same::type>::value, "");
-	assert(res1 == 1324997410816LL);
+auto res1 = std::lcm(static_cast(1234), INT32_MIN);
+(void)std::lcm(INT_MIN, 2UL);	// this used to trigger UBSAN
+static_assert(std::is_same_v, "");
+assert(res1 == 1324997410816LL);
 }
 }
Index: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
===
--- test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
+++ test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
@@ -16,8 +16,10 @@
 
 #include 
 #include 
+#include 
+#include 
 #include // for rand()
-#include 
+#include 
 
 constexpr struct {
   int x;
@@ -36,21 +38,24 @@
 
 
 template 
-constexpr bool test0(Input1 in1, Input2 in2, Output out)
+constexpr bool test0(int in1, int in2, int out)
 {
-static_assert((std::is_same::value), "" );
-static_assert((std::is_same::value), "" );
-return out == std::gcd(in1, in2) ? true : (std::abort(), false);
+auto value1 = static_cast(in1);
+auto value2 = static_cast(in2);
+static_assert(std::is_same_v, "");
+static_assert(std::is_same_v, "");
+assert(static_cast(out) == std::gcd(value1, value2));
+return true;
 }
 
 
 template 
 constexpr bool do_test(int = 0)
 {
-using S1 = typename std::make_signed::type;
-using S2 = typename std::make_signed::type;
-using U1 = typename std::make_unsigned::type;
-using U2 = typename std::make_unsigned::type;
+using S1 = std::make_signed_t;
+using S2 = std::make_signed_t;
+using U1 = std::make_unsigned_t;
+using U2 = std::make_unsigned_t;
 bool accumulate = true;
 for (auto TC : Cases) {
 { // Test 

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp:42
+constexpr bool test0(int in1, int in2, int out)
 {
+static_assert(std::is_same EricWF wrote:
> > Nit but this seems much cleaner and more readable without the casts.
> > 
> > ```
> > auto value1 = static_cast(in1);
> > auto value2 = static_cast(in2);
> > static_assert(std::is_same_v, 
> > "");
> > static_assert(std::is_same_v, 
> > "");
> > assert(static_cast(out) == std::gcd(value1, value2));
> > return true;
> > ```
> > 
> > Also just use `assert` instead of `std::abort()`
> I was under the impression the abort dance was because assert isn't constexpr.
`assert` is constexpr for the exact same reason the `abort` dance was constexpr.

And I think somebody else was under the same misapprehension when they wrote 
this test.


https://reviews.llvm.org/D32309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal added inline comments.



Comment at: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp:42
+constexpr bool test0(int in1, int in2, int out)
 {
+static_assert(std::is_same Nit but this seems much cleaner and more readable without the casts.
> 
> ```
> auto value1 = static_cast(in1);
> auto value2 = static_cast(in2);
> static_assert(std::is_same_v, "");
> static_assert(std::is_same_v, "");
> assert(static_cast(out) == std::gcd(value1, value2));
> return true;
> ```
> 
> Also just use `assert` instead of `std::abort()`
I was under the impression the abort dance was because assert isn't constexpr.


https://reviews.llvm.org/D32309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Eric Fiselier via Phabricator via cfe-commits]

EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.

LGTM, preferably with the suggested cleanups.




Comment at: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp:146
+auto res = std::gcd(static_cast(1234), INT32_MIN);
+static_assert(std::is_same>::value, "");
 assert(res == 2);

BillyONeal wrote:
> EricWF wrote:
> > `std::common_type` here please. This test runs in C++03 where we don't have 
> > `std::foo_t`.
> C++17 library features in C++03?!? :(
> 
> OK, will fix.
> 
Sorry my bad. This test is truely C++17 only. I missed the `// UNSUPPORTED` 
line originally. Feel free to run wild with C++17 features.



Comment at: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp:42
+constexpr bool test0(int in1, int in2, int out)
 {
+static_assert(std::is_same, "");
static_assert(std::is_same_v, "");
assert(static_cast(out) == std::gcd(value1, value2));
return true;
```

Also just use `assert` instead of `std::abort()`



Comment at: test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp:41
 {
-static_assert((std::is_same::value), "" );
-static_assert((std::is_same::value), "" );
-return out == std::lcm(in1, in2) ? true : (std::abort(), false);
+static_assert(std::is_same::value, "");

Same suggestion as on the above test.


https://reviews.llvm.org/D32309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal updated this revision to Diff 98205.
BillyONeal added a comment.

Resolved CR comments


https://reviews.llvm.org/D32309

Files:
  test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
  test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp

Index: test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
===
--- test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
+++ test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
@@ -11,12 +11,14 @@
 // 
 
 // template
-// constexpr common_type_t<_M,_N> gcd(_M __m, _N __n)
+// constexpr common_type_t<_M,_N> lcm(_M __m, _N __n)
 
 #include 
 #include 
+#include 
+#include 
 #include 
-#include 
+#include 
 
 constexpr struct {
   int x;
@@ -34,11 +36,19 @@
 };
 
 template 
-constexpr bool test0(Input1 in1, Input2 in2, Output out)
+constexpr bool test0(int in1, int in2, int out)
 {
-static_assert((std::is_same::value), "" );
-static_assert((std::is_same::value), "" );
-return out == std::lcm(in1, in2) ? true : (std::abort(), false);
+static_assert(std::is_same::value, "");
+static_assert(std::is_same::value, "");
+const bool result = static_cast(out) ==
+std::lcm(static_cast(in1), static_cast(in2));
+if (!result) {
+std::abort();
+}
+
+return result;
 }
 
 
@@ -101,15 +111,15 @@
 assert(do_test(non_cce));
 assert(do_test(non_cce));
 
-static_assert(do_test< int8_t>(), "");
-static_assert(do_test(), "");
-static_assert(do_test(), "");
-static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
 
-assert(do_test< int8_t>(non_cce));
-assert(do_test(non_cce));
-assert(do_test(non_cce));
-assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
 
 static_assert(do_test(), "");
 static_assert(do_test(), "");
@@ -131,9 +141,9 @@
 
 //  LWG#2837
 {
-auto res1 = std::lcm((int64_t)1234, (int32_t)-2147483648);
-(void) std::lcm(INT_MIN, 2);	// this used to trigger UBSAN
-static_assert( std::is_same::type>::value, "");
-	assert(res1 == 1324997410816LL);
+auto res1 = std::lcm(static_cast(1234), INT32_MIN);
+(void)std::lcm(INT_MIN, 2UL);	// this used to trigger UBSAN
+static_assert(std::is_same::value, "");
+assert(res1 == 1324997410816LL);
 }
 }
Index: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
===
--- test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
+++ test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
@@ -16,8 +16,10 @@
 
 #include 
 #include 
+#include 
+#include 
 #include // for rand()
-#include 
+#include 
 
 constexpr struct {
   int x;
@@ -36,11 +38,19 @@
 
 
 template 
-constexpr bool test0(Input1 in1, Input2 in2, Output out)
+constexpr bool test0(int in1, int in2, int out)
 {
-static_assert((std::is_same::value), "" );
-static_assert((std::is_same::value), "" );
-return out == std::gcd(in1, in2) ? true : (std::abort(), false);
+static_assert(std::is_same::value, "");
+static_assert(std::is_same::value, "");
+const bool result = static_cast(out) ==
+std::gcd(static_cast(in1), static_cast(in2));
+if (!result) {
+std::abort();
+}
+
+return result;
 }
 
 
@@ -103,15 +113,15 @@
 assert(do_test(non_cce));
 assert(do_test(non_cce));
 
-static_assert(do_test< int8_t>(), "");
-static_assert(do_test(), "");
-static_assert(do_test(), "");
-static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
 
-assert(do_test< int8_t>(non_cce));
-assert(do_test(non_cce));
-assert(do_test(non_cce));
-assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
 
 static_assert(do_test(), "");
 static_assert(do_test(), "");
@@ -133,8 +143,8 @@
 
 //  LWG#2837
 {
-auto res = std::gcd((int64_t)1234, (int32_t)-2147483648);
-static_assert( std::is_same

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal added inline comments.



Comment at: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp:146
+auto res = std::gcd(static_cast(1234), INT32_MIN);
+static_assert(std::is_same>::value, "");
 assert(res == 2);

EricWF wrote:
> `std::common_type` here please. This test runs in C++03 where we don't have 
> `std::foo_t`.
C++17 library features in C++03?!? :(

OK, will fix.



https://reviews.llvm.org/D32309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp:146
+auto res = std::gcd(static_cast(1234), INT32_MIN);
+static_assert(std::is_same>::value, "");
 assert(res == 2);

`std::common_type` here please. This test runs in C++03 where we don't have 
`std::foo_t`.


https://reviews.llvm.org/D32309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Casey Carter via Phabricator via cfe-commits]

CaseyCarter added inline comments.



Comment at: test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp:144
+auto res1 = std::lcm(static_cast(1234), INT32_MIN);
+(void)std::lcm(INT_MIN, 2UL);  // this used to trigger UBSAN
+static_assert(std::is_same::value, "");

`INT_MIN` is in `` .


https://reviews.llvm.org/D32309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal added a comment.

Hi folks, any update on this or is just fixing @rsmith's comment OK?

Thanks!


https://reviews.llvm.org/D32309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal added inline comments.



Comment at: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp:46
+std::gcd(static_cast(0), static_cast(0)))>::value, "");
+const bool result = static_cast>(out) ==
+std::gcd(static_cast(in1), static_cast(in2));

rsmith wrote:
> Is there a reason to recompute the type here rather than using `Output`?
Hmm... I think I was concerned about it looking like the cast o the output type 
was an assertion that the return type was in fact of that type (which isn't 
true). But that should be taken care of by the previous static_asserts.


https://reviews.llvm.org/D32309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp:46
+std::gcd(static_cast(0), static_cast(0)))>::value, "");
+const bool result = static_cast>(out) ==
+std::gcd(static_cast(in1), static_cast(in2));

Is there a reason to recompute the type here rather than using `Output`?


https://reviews.llvm.org/D32309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32309: [libcxx] [test] Resolve compiler warnings in LCM/GCD tests

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal updated this revision to Diff 96231.
BillyONeal added a comment.

Stephan had some code review comments :)


https://reviews.llvm.org/D32309

Files:
  test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
  test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp

Index: test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
===
--- test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
+++ test/std/numerics/numeric.ops/numeric.ops.lcm/lcm.pass.cpp
@@ -11,12 +11,13 @@
 // 
 
 // template
-// constexpr common_type_t<_M,_N> gcd(_M __m, _N __n)
+// constexpr common_type_t<_M,_N> lcm(_M __m, _N __n)
 
 #include 
 #include 
+#include 
 #include 
-#include 
+#include 
 
 constexpr struct {
   int x;
@@ -34,21 +35,29 @@
 };
 
 template 
-constexpr bool test0(Input1 in1, Input2 in2, Output out)
+constexpr bool test0(int in1, int in2, int out)
 {
-static_assert((std::is_same::value), "" );
-static_assert((std::is_same::value), "" );
-return out == std::lcm(in1, in2) ? true : (std::abort(), false);
+static_assert(std::is_same::value, "");
+static_assert(std::is_same::value, "");
+const bool result = static_cast>(out) ==
+std::lcm(static_cast(in1), static_cast(in2));
+if (!result) {
+std::abort();
+}
+
+return result;
 }
 
 
 template 
 constexpr bool do_test(int = 0)
 {
-using S1 = typename std::make_signed::type;
-using S2 = typename std::make_signed::type;
-using U1 = typename std::make_unsigned::type;
-using U2 = typename std::make_unsigned::type;
+using S1 = std::make_signed_t;
+using S2 = std::make_signed_t;
+using U1 = std::make_unsigned_t;
+using U2 = std::make_unsigned_t;
 bool accumulate = true;
 for (auto TC : Cases) {
 { // Test with two signed types
@@ -101,15 +110,15 @@
 assert(do_test(non_cce));
 assert(do_test(non_cce));
 
-static_assert(do_test< int8_t>(), "");
-static_assert(do_test(), "");
-static_assert(do_test(), "");
-static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
+static_assert(do_test(), "");
 
-assert(do_test< int8_t>(non_cce));
-assert(do_test(non_cce));
-assert(do_test(non_cce));
-assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
+assert(do_test(non_cce));
 
 static_assert(do_test(), "");
 static_assert(do_test(), "");
@@ -131,9 +140,9 @@
 
 //  LWG#2837
 {
-auto res1 = std::lcm((int64_t)1234, (int32_t)-2147483648);
-(void) std::lcm(INT_MIN, 2);	// this used to trigger UBSAN
-static_assert( std::is_same::type>::value, "");
+auto res1 = std::lcm(static_cast(1234), INT32_MIN);
+(void)std::lcm(INT_MIN, 2UL);	// this used to trigger UBSAN
+static_assert(std::is_same::value, "");
 	assert(res1 == 1324997410816LL);
 }
 }
Index: test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
===
--- test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
+++ test/std/numerics/numeric.ops/numeric.ops.gcd/gcd.pass.cpp
@@ -16,8 +16,9 @@
 
 #include 
 #include 
+#include 
 #include // for rand()
-#include 
+#include 
 
 constexpr struct {
   int x;
@@ -36,21 +37,29 @@
 
 
 template 
-constexpr bool test0(Input1 in1, Input2 in2, Output out)
+constexpr bool test0(int in1, int in2, int out)
 {
-static_assert((std::is_same::value), "" );
-static_assert((std::is_same::value), "" );
-return out == std::gcd(in1, in2) ? true : (std::abort(), false);
+static_assert(std::is_same::value, "");
+static_assert(std::is_same::value, "");
+const bool result = static_cast>(out) ==
+std::gcd(static_cast(in1), static_cast(in2));
+if (!result) {
+std::abort();
+}
+
+return result;
 }
 
 
 template 
 constexpr bool do_test(int = 0)
 {
-using S1 = typename std::make_signed::type;
-using S2 = typename std::make_signed::type;
-using U1 = typename std::make_unsigned::type;
-using U2 = typename std::make_unsigned::type;
+using S1 = std::make_signed_t;
+using S2 = std::make_signed_t;
+using U1 = std::make_unsigned_t;
+