std::abs should not return double (2735)

[zoecarver]

Summary

Implement 2735 by checking to make sure argument both is_integral and not is_unsigned.

Question
The issue describes abs returning an int always but for larger int types this would not work. I assume checking to make sure they are integral types is okay, but I am not sure.

TODO

• write tests (make sure unsigned ints fail)

[zoecarver]

Use proper format for diff.

[zoecarver]

Add tests.

[mclow.lists]

The title of https://wg21.link/LWG2735 is "std::abs(short), std::abs(signed char) and others should return int instead of double in order to be compatible with C++98 and C"

But that's not what you've implemented.
In your code std::abs(short) returns a short, etc.

Also, the tests belong in test/std, not test/libcxx, because they're testing "standard" behavior, not libc++-specific behavior.

test/libcxx/numerics/c.math/abs.fail.cpp

20 ↗

(On Diff #185441)

You should use the error checking bits of the test suite // expected-error... to check that this is generating the error that you want.

[zoecarver]

Thanks for the review. Looks like clang already returns ints instead of shorts & chars and properly returns longs, etc so I guess this isn't really an issue.

Is there a way to cross it off here?

I will update the patch and remove the added abs function (leaving the tests). Sound good?

[zoecarver]

Remove additional abs declaration. Fix tests.

[mclow.lists]

test/std/numerics/c.math/abs.fail.cpp

9

Is this the right file to include - or should it be <cmath>?

18

As of yesterday, we always write main as int main (int, char**)

test/std/numerics/c.math/abs.pass.cpp

20

I would add a second template parameter here, that is the expected return type.
Some thing like:

template <class Source, class Result>
void test_abs()
{
    T neg_val = -5;
    T pos_val = 5;
    Result res = 5;

    ASSERT_SAME_TYPE(Result, decltype(std::abs(T));

    assert(std::abs(neg_val) == res);
    assert(std::abs(pos_val) == res);
}

51

By adding the second template parameter to test_abs, this line becomes redundant.

[mclow.lists]

test/std/numerics/c.math/abs.pass.cpp

32

How do you assure that this is bigger than numeric_limits<int>::max()?

[zoecarver]

Fix review comments.

[mclow.lists]

Almost there - down to nits

test/std/numerics/c.math/abs.pass.cpp

29

We don't usually put assert messages into tests.
The assert will give you file and line, and that's enough.

36

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.

50

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.

[mclow.lists]

test/std/numerics/c.math/abs.pass.cpp

49

One more nit - this test will fail on systems where char is unsigned.

[zoecarver]

test/std/numerics/c.math/abs.pass.cpp

50

@mclow.lists If int64/int128 are "upgraded" to ints, won't they lose the better half of their data?

[zoecarver]

Check off in cxx1z status.

[EricWF]

test/std/numerics/c.math/abs.fail.cpp

14

No need to test this here. There should be other tests that ensure <cmath> define _LIBCPP_VERSION

18

int main() is fine. Same with omitting return 0;.

22

More tests for unsigned char, unsigned short, unsigned long, and unsigned long long?

test/std/numerics/c.math/abs.pass.cpp

17

Same comment here.

28

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 on T.

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

41

int main() is fine. Same with omitting return 0;.

46

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>();

55

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.

[mclow.lists]

test/std/numerics/c.math/abs.fail.cpp

22

No. std::abs is not defined for unsigned types.

test/std/numerics/c.math/abs.pass.cpp

28

I suggested that Zoe be explicit here.

41

Not with the new freestanding stuff that Louis is working on

55

this is true.

[mclow.lists]

test/std/numerics/c.math/abs.pass.cpp

28

In particular, we want to be sure that std::abs(char) returns int, not char, which can be promoted to an int.

[zoecarver]

test/std/numerics/c.math/abs.pass.cpp

28

I will play around with this a bit and see what works. Is there a template for integral promotion?

41

@EricWF Is this preferred?

46

Will do. C++ templates never cease to amaze me :D

[zoecarver]

test/std/numerics/c.math/abs.pass.cpp

28

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;
};

[EricWF]

test/std/numerics/c.math/abs.fail.cpp

22

Right, This is a failure test. Unsigned types should fail.

test/std/numerics/c.math/abs.pass.cpp

28

@mclow.lists Only if char is signed. But yes.

41

Fudge me, really? OK. Nevermind then.

41

Woops. My bad. I didn't know we are switching how we declare main. Ignore me.

[zoecarver]

test/std/numerics/c.math/abs.pass.cpp

28

I think I am going to leave it the way it was originally for now (I like the explicitness of it anyway).

41

Oh, didn't see your previous comment -- sorry.

55

I fixed this by expecting int64_t .

[zoecarver]

Fix based on review

[mclow.lists]

I think this is good to go!

[zoecarver]

Thanks @mclow.lists! I don't have commit access, would you mind committing it for me?

[zoecarver]

• update the expected error from calling std::abs on unsigned char and short (does anyone know why those types got a specialization here?).

[mclow.lists]

libcxx/test/std/numerics/c.math/abs.pass.cpp

51 ↗

(On Diff #214707)

I think these tests are nonportable. The assumption here is that int32_t (and others) are smaller than (or the same size as) int, and int64_t is larger.

Maybe you want a little type trait (untested)

template <typename T> struct smaller_than_int {
    typedef typename std::conditional<sizeof(T) < sizeof(int), int, T>::type type;
};

and then write test_abs<std::int8_t, smaller_than_int <std::int8_t>();

[zoecarver]

• make test portable by checking size of R.

[zoecarver]

libcxx/test/std/numerics/c.math/abs.pass.cpp

51 ↗

(On Diff #214707)

I added this to test_abs so we always use it.

[mclow.lists]

libcxx/test/std/numerics/c.math/abs.pass.cpp

51 ↗

(On Diff #214707)

You should pull this out of this function, and put it into the template parameter list, because it shouldn't apply to floating point types.

[zoecarver]

• add correct_size_int type trait so Result is only changed based on the size of integral types (not floating points)

[mclow.lists]

LGTM.

[zoecarver]

Committed as rL369394.

[zoecarver]

• fix on systems where char is unsigned.

[zoecarver]

On systems where sizeof(long) == sizeof(int) the overload std::abs(long int) will still be picked. Therefore we must ensure that we assert the correct return type. This update fixes the current issue, but I don't think it is a perfect solution. For example if sizeof(short int) == sizeof(int) it would still be upgraded to type int and the test would fail.

[zoecarver]

Given that long int and long long int will never be smaller than int we could remove the correct_size_int type trait from those calls and re-add the <= check. Maybe that would be the best solution.

[zoecarver]

Remove correct int type trait (as it is not needed and adds complexity)

[zoecarver]

• re-add correct_size_int helper for cstdint types

test/std/numerics/c.math/abs.fail.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <cstdlib>
+
+#include "test_macros.h"
+
+#ifndef _LIBCPP_VERSION
+#error _LIBCPP_VERSION not defined
+#endif
+
+int main()
+{
+    unsigned int ui = -5;
+    abs(ui); // expected-error {{Call to 'abs' is ambiguous}}
+    
+    return 0;
+}
+

test/std/numerics/c.math/abs.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <math.h>
+#include <assert.h>
+
+#include "test_macros.h"
+
+#ifndef _LIBCPP_VERSION
+#error _LIBCPP_VERSION not defined
+#endif
+
+template <class T>
+void test_abs()
+{
+    T val = -5;
+    T pos_val = 5;
+    
+    assert(abs(val) == 5);
+    assert(abs(pos_val) == 5);
+    
+    ASSERT_SAME_TYPE(decltype(abs(val)), T);
+}
+
+void test_big()
+{
+    long long int big_value = 9223372036854775800; // a value to big for ints to store
+    long long int negative_big_value = -big_value;
+    assert(abs(negative_big_value) == big_value); // make sure it doesnt get casted to a smaller type
+}
+
+int main()
+{
+    test_abs<int>();
+    test_abs<long int>();
+    test_abs<long long int>();
+    test_abs<long double>();
+    test_abs<double>();
+    test_abs<float>();
+    test_abs<short int>();
+    test_abs<char>();
+    
+    test_big();
+    
+    return 0;
+}
+