[libcxx-commits] [PATCH] D57778: std::abs should not return double (2735)

[Marshall Clow via Phabricator via libcxx-commits]

mclow.lists added a comment.

Almost there - down to nits



================
Comment at: test/std/numerics/c.math/abs.pass.cpp:28
+                     
+    assert(std::abs(neg_val) == res && "Absolute value returned incorrect value");
+    assert(std::abs(pos_val) == res && "Absolute value returned incorrect value");
----------------
We don't usually put assert messages into tests.
The assert will give you file and line, and that's enough.



================
Comment at: test/std/numerics/c.math/abs.pass.cpp:35
+    long long int big_value = std::numeric_limits<long long int>::max(); // a value to big for ints to store
+    long long int negative_big_value = -big_value;
+    assert(std::abs(negative_big_value) == big_value); // make sure it doesnt get casted to a smaller type
----------------
This test won't do what you want on a machine where int/long/long long are all the same size.
Fortunately, they're pretty rare.


================
Comment at: test/std/numerics/c.math/abs.pass.cpp:49
+    test_abs<char, int>();
+    
+    test_big();
----------------
Any other integral types you want to try?  `signed char` leaps to mind. (Yes, it's a different type than `char`)
`bool` (sadly) is integral, but (fortunately) not signed.

Also, I would sort these differently. Put the floating point ones at the end. (nit)
    char/signed char/short/int/long/long long
then
    int8_t/int16_t/int32_t/int64_t/__int128_t
and finally
    float/double/long double

Not sure about the sized integers - I think you should leave them off for now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57778/new/

https://reviews.llvm.org/D57778