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

52

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

52

I added this to test_abs so we always use it.

[mclow.lists]

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

52

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.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 <cmath>
+
+#include "test_macros.h"
+
+int main(int, char**)
+{
+    unsigned int ui = -5;
+    ui = std::abs(ui); // expected-error {{call to 'abs' is ambiguous}}
+
+    unsigned char uc = -5;
+    uc = std::abs(uc); // expected-error {{taking the absolute value of unsigned type 'unsigned char' has no effect}}
+
+    unsigned short us = -5;
+    us = std::abs(us); // expected-error {{taking the absolute value of unsigned type 'unsigned short' has no effect}}
+
+    unsigned long ul = -5;
+    ul = std::abs(ul); // expected-error {{call to 'abs' is ambiguous}}
+
+    unsigned long long ull = -5;
+    ull = ::abs(ull); // expected-error {{call to 'abs' is ambiguous}}
+
+    return 0;
+}
+

libcxx/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 <assert.h>
+#include <cmath>
+#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;
+};
+
+template <class Source, class Result>
+void test_abs()
+{
+    Source neg_val = -5;
+    Source pos_val = 5;
+    Result res = 5;
+
+    ASSERT_SAME_TYPE(decltype(std::abs(neg_val)), Result);
+
+    assert(std::abs(neg_val) == res);
+    assert(std::abs(pos_val) == res);
+}
+
+void test_big()
+{
+    long long int big_value = std::numeric_limits<long long int>::max(); // a value to big for ints to store
+    long long int negative_big_value = -big_value;
+    assert(std::abs(negative_big_value) == big_value); // make sure it doesnt get casted to a smaller type
+}
+
+int main(int, char**)
+{
+    test_abs<short int, typename correct_size_int<short int>::type>();
+    test_abs<char, typename correct_size_int<char>::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>();
+    test_abs<std::int32_t, typename correct_size_int<std::int32_t>::type>();
+    test_abs<std::int64_t, typename correct_size_int<std::int64_t>::type>();
+
+    test_abs<long double, long double>();
+    test_abs<double, double>();
+    test_abs<float, float>();
+
+    test_big();
+
+    return 0;
+}
+

libcxx/www/cxx1z_status.html

<!--   <I>Note: "NAD" means that the issue was deemed "Not a defect"</I> -->
   <tr><td><a href="https://wg21.link/LWG2694">2694</a></td><td>Application of LWG 436 accidentally deleted definition of "facet"</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2696">2696</a></td><td>Interaction between make_shared and enable_shared_from_this is underspecified</td><td>Issaquah</td><td>Nothing to do</td></tr>
   <tr><td><a href="https://wg21.link/LWG2699">2699</a></td><td>Missing restriction in [numeric.requirements]</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2712">2712</a></td><td>copy_file(from, to, ...) has a number of unspecified error conditions</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2722">2722</a></td><td>equivalent incorrectly specifies throws clause</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2729">2729</a></td><td>Missing SFINAE on std::pair::operator=</td><td>Issaquah</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG2732">2732</a></td><td>Questionable specification of path::operator/= and path::append</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2733">2733</a></td><td>[fund.ts.v2] gcd / lcm and bool</td><td>Issaquah</td><td>Complete</td></tr>
-  <tr><td><a href="https://wg21.link/LWG2735">2735</a></td><td>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</td><td>Issaquah</td><td></td></tr>
+  <tr><td><a href="https://wg21.link/LWG2735">2735</a></td><td>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</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2736">2736</a></td><td>nullopt_t insufficiently constrained</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2738">2738</a></td><td>is_constructible with void types</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2739">2739</a></td><td>Issue with time_point non-member subtraction with an unsigned duration</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2740">2740</a></td><td>constexpr optional<T>::operator-></td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2742">2742</a></td><td>Inconsistent string interface taking string_view</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2744">2744</a></td><td>any's in_place constructors</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2745">2745</a></td><td>[fund.ts.v2] Implementability of LWG 2451</td><td>Issaquah</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG2747">2747</a></td><td>Possibly redundant std::move in [alg.foreach]</td><td>Issaquah</td><td>Complete</td></tr>