[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:21
+    std::abs(ui); // expected-error {{call to 'abs' is ambiguous}}
+    
+    return 0;
----------------
mclow.lists wrote:
> EricWF wrote:
> > More tests for `unsigned char`, `unsigned short`, `unsigned long`, and `unsigned long long`?
> No. std::abs is not defined for unsigned types.
> 
Right, This is a failure test. Unsigned types should fail.


================
Comment at: test/std/numerics/c.math/abs.pass.cpp:27
+    
+    ASSERT_SAME_TYPE(decltype(std::abs(neg_val)), R);
+                     
----------------
zoecarver wrote:
> zoecarver wrote:
> > 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? 
> Do either of you know how to do something like this?
> 
> ```
> template <typename T, typename U>
> struct upgrade_intergal
> {
>     using type = typename std::conditional<sizeof(T) <= sizeof(U), U, upgrade_intergal<T, long U>>::type;
> };
> ```
@mclow.lists Only if `char` is signed. But yes.


================
Comment at: test/std/numerics/c.math/abs.pass.cpp:40
+
+int main(int, char**)
+{
----------------
zoecarver wrote:
> 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?
Fudge me, really? OK. Nevermind then.


================
Comment at: test/std/numerics/c.math/abs.pass.cpp:40
+
+int main(int, char**)
+{
----------------
EricWF wrote:
> zoecarver wrote:
> > 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?
> Fudge me, really? OK. Nevermind then.
Woops. My bad. I didn't know we are switching how we declare main. Ignore me.


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

https://reviews.llvm.org/D57778