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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 3 inline comments as done.
zoecarver added inline comments.


================
Comment at: test/std/numerics/c.math/abs.pass.cpp:27
+    
+    ASSERT_SAME_TYPE(decltype(std::abs(neg_val)), R);
+                     
----------------
mclow.lists wrote:
> mclow.lists wrote:
> > EricWF wrote:
> > > 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);`
> > I suggested that Zoe be explicit here.
> In particular, we want to be sure that `std::abs(char)` returns `int`, not `char`, which can be promoted to an `int`.
> 
I will play around with this a bit and see what works. Is there a template for integral promotion? 


================
Comment at: test/std/numerics/c.math/abs.pass.cpp:40
+
+int main(int, char**)
+{
----------------
mclow.lists wrote:
> EricWF wrote:
> > `int main()` is fine. Same with omitting `return 0;`.
> Not with the new freestanding stuff that Louis is working on
@EricWF Is this preferred?


================
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
+    
----------------
EricWF wrote:
> 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>();
> ```
Will do. C++ templates never cease to amaze me :D


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

https://reviews.llvm.org/D57778