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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
Comment at: test/std/numerics/c.math/abs.fail.cpp:13
+
+#ifndef _LIBCPP_VERSION
+#error _LIBCPP_VERSION not defined
----------------
No need to test this here. There should be other tests that ensure `<cmath>` define `_LIBCPP_VERSION`


================
Comment at: test/std/numerics/c.math/abs.fail.cpp:17
+
+int main(int, char**)
+{
----------------
`int main()` is fine. Same with omitting `return 0;`.


================
Comment at: test/std/numerics/c.math/abs.fail.cpp:21
+    std::abs(ui); // expected-error {{call to 'abs' is ambiguous}}
+    
+    return 0;
----------------
More tests for `unsigned char`, `unsigned short`, `unsigned long`, and `unsigned long long`?


================
Comment at: test/std/numerics/c.math/abs.pass.cpp:16
+
+#ifndef _LIBCPP_VERSION
+#error _LIBCPP_VERSION not defined
----------------
Same comment here.


================
Comment at: test/std/numerics/c.math/abs.pass.cpp:27
+    
+    ASSERT_SAME_TYPE(decltype(std::abs(neg_val)), R);
+                     
----------------
I think this test for the return type can be simplified. We expect `std::abs(T)` to return `U` where `U` is the result of [integral promotion](https://en.cppreference.com/w/cpp/language/implicit_conversion)  on `T`.

Instead of hard-coding the expected type, we can calculate it using the expression `using R = decltype(+pos_value);`


================
Comment at: test/std/numerics/c.math/abs.pass.cpp:40
+
+int main(int, char**)
+{
----------------
`int main()` is fine. Same with omitting `return 0;`.


================
Comment at: test/std/numerics/c.math/abs.pass.cpp:45
+    test_abs<signed char, int>();
+    if (std::is_signed<char>::value) test_abs<char, int>(); // sometimes chars are unsigned
+    
----------------
This will still cause the instantiation of `test_abs<char, int>()` when false. Which will make the test not compile.

Maybe this instead?
```
test_abs<std::conditional<std::is_signed<char>::value, char, signed char>::type, int>();
```


================
Comment at: test/std/numerics/c.math/abs.pass.cpp:54
+    test_abs<std::int32_t, int>();
+    test_abs<std::int64_t, long long>();
+    
----------------
This is non-portable. AFAIK there is no reason why `int64_t` can't be `long` on 64 bit platforms. 
Using the integral promotion approach avoids this.


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

https://reviews.llvm.org/D57778