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

17 ↗

(On Diff #185488)

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

9 ↗

(On Diff #185473)

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

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

19 ↗

(On Diff #185488)

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

50 ↗

(On Diff #185488)

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

[mclow.lists]

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

31 ↗

(On Diff #185488)

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

28 ↗

(On Diff #185584)

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

35 ↗

(On Diff #185584)

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.

49 ↗

(On Diff #185584)

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

48 ↗

(On Diff #185584)

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

[zoecarver]

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

49 ↗

(On Diff #185584)

@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

13 ↗

(On Diff #185631)

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

17 ↗

(On Diff #185631)

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

21 ↗

(On Diff #185631)

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

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

16 ↗

(On Diff #185631)

Same comment here.

27 ↗

(On Diff #185631)

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

40 ↗

(On Diff #185631)

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

45 ↗

(On Diff #185631)

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

54 ↗

(On Diff #185631)

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

21 ↗

(On Diff #185631)

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

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

27 ↗

(On Diff #185631)

I suggested that Zoe be explicit here.

40 ↗

(On Diff #185631)

Not with the new freestanding stuff that Louis is working on

54 ↗

(On Diff #185631)

this is true.

[mclow.lists]

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

27 ↗

(On Diff #185631)

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

27 ↗

(On Diff #185631)

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

40 ↗

(On Diff #185631)

@EricWF Is this preferred?

45 ↗

(On Diff #185631)

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

[zoecarver]

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

27 ↗

(On Diff #185631)

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

21 ↗

(On Diff #185631)

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

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

27 ↗

(On Diff #185631)

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

40 ↗

(On Diff #185631)

Fudge me, really? OK. Nevermind then.

40 ↗

(On Diff #185631)

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

27 ↗

(On Diff #185631)

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

40 ↗

(On Diff #185631)

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

54 ↗

(On Diff #185631)

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

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

I added this to test_abs so we always use it.

[mclow.lists]

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

51

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

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

 #include <cstdint>
 #include <type_traits>
 
 #include "test_macros.h"
 
 template<class T>
 struct correct_size_int
 {
-    typedef typename std::conditional<sizeof(T) <= sizeof(int), int, T>::type type;
+    typedef typename std::conditional<sizeof(T) < sizeof(int), int, T>::type type;
 };
 
 template <class Source, class Result>
 void test_abs()
 {
     Source neg_val = -5;
     Source pos_val = 5;
     Result res = 5;

 {
     // On some systems char is unsigned.
     // If that is the case, we should just test signed char twice.
     typedef typename std::conditional<
         std::is_signed<char>::value, char, signed char
     >::type SignedChar;
 
     test_abs<short int, typename correct_size_int<short int>::type>();
     test_abs<SignedChar, typename correct_size_int<SignedChar>::type>();
     test_abs<signed char, typename correct_size_int<signed char>::type>();
 
     test_abs<int, typename correct_size_int<int>::type>();
     test_abs<long int, typename correct_size_int<long int>::type>();
     test_abs<long long int, typename correct_size_int<long long int>::type>();
 
     test_abs<std::int8_t, typename correct_size_int<std::int8_t>::type>();
     test_abs<std::int16_t, typename correct_size_int<std::int16_t>::type>();