<cmath>: signbit() misses overloads for integer types

[PhilipDeegan]

https://developercommunity.visualstudio.com/content/problem/923187/stdsignbit-misses-overloads-for-integer-types.html#reply-923210

jbeder/yaml-cpp#823

[StephanTLavavej]

In addition to DevCom-923187, this was previously reported as Microsoft-internal VSO-521345, VSO-153341, VSO-194989, VSO-294538, and VSO-531712.

We resolved those bugs, saying that when LWG-1327 was resolved, the intent was to support floating-point types only. There's a comment in <cmath> about this:

STL/stl/inc/cmath

Line 674 in fffbd8f

// The "classification/comparison functions" (fpclassify(), etc.) are exempt, LWG-1327

However, this issue keeps being encountered by users, and I do not see clear language in the current WP justifying our implementation. WG21-N4849 [cmath.syn]/2 simply says "For each set of overloaded functions within <cmath>, with the exception of abs, there shall be additional overloads sufficient to ensure:" and signbit() etc. is a set of overloaded functions within <cmath>.

I believe that either this is a bug in our implementation, or it's a defect in the Standard that needs an LWG issue to be filed (specifically mentioning the "classification/comparison functions" alongside abs).

[BillyONeal]

I've asked on the LEWG mailing list a few times about this (choosing to resolve in favor of providing the overloads or not is an LEWG rather than LWG question) but haven't gotten any response. I wanted to write a paper about it but wanted to wait until the "mandating the standard library" papers got merged.

[BillyONeal]

It looks like WG21-P0175 may have broken the reasoning we use for LWG-1327 -- the old wording said that the "sufficient additional overloads only apply to functions from the C standard library" and in the C standard library they are macros. Now we explicitly declare them as functions...

[timsong-cpp]

LWG-1327 is resolved by Motion 27 at Rapperswil 2010. That applied the wording for US136 from WG21-N3102. The description preceding the wording says

Each macro is better described in C++ as three overloads, with operand(s) of type float,
double, and long double. Moreover, as with the math functions described in this same section, each of the functions has "sufficient additional overloads" to simulate the effect of the C type-generic functions.

This was also presented to WG14 at the same time; WG14-N1459 makes clear that the C comparison macros are intended to support heterogeneous floating-point types. That's only realizable in the C++ wording if "sufficient additional overloads" apply.

[StephanTLavavej]

We should actually tag this as LEWG issue needed because this is a library design issue.

[StephanTLavavej]

Closed #908 as a duplicate of this bug; it received 6 thumbs-up in addition to the submitter.

[BillyONeal]

One thing I forgot to note there: unlike the rest of the fpclassify family, signbit can do something meaningful for integers so we might want to consider it separately (as opposed to isnan/isfinite et al. which are almost certainly mistakes to call on integers).

…

________________________________ From: Stephan T. Lavavej <notifications@github.com> Sent: Saturday, June 20, 2020 07:06 PM To: microsoft/STL <STL@noreply.github.com> Cc: Billy O'Neal (VC LIBS) <bion@microsoft.com>; Comment <comment@noreply.github.com> Subject: Re: [microsoft/STL] <cmath>: signbit() misses overloads for integer types (#519) Closed #908<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2FSTL%2Fissues%2F908&data=02%7C01%7Cbion%40microsoft.com%7C816c466ffca34e5be26c08d81587c8b7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637283020214117113&sdata=p9URfQhgYB1JcKgq0EHFI%2F9CSAPTnwpGV8ux2OPrLdk%3D&reserved=0> as a duplicate of this bug; it received 6 thumbs-up in addition to the submitter. — You are receiving this because you commented. Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2FSTL%2Fissues%2F519%23issuecomment-647068256&data=02%7C01%7Cbion%40microsoft.com%7C816c466ffca34e5be26c08d81587c8b7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637283020214127108&sdata=8RA%2FZhqt3D7NUn9pbqTw9nhaf7%2FDht%2BeRcqgDmfpiOY%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAALZF37AAKZF3RLHV2UGNWTRXVTMHANCNFSM4KYVS6RA&data=02%7C01%7Cbion%40microsoft.com%7C816c466ffca34e5be26c08d81587c8b7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637283020214127108&sdata=bu6oA6IyEc0ioyCh%2BWsSqUYEE58zGlQptYse9ebELrw%3D&reserved=0>.

[TedLyngmo]

@BillyONeal The usefulness of std::is_integral overloads for std::isnan etc. (which I assume will just return false as-if return isnan(static_cast<double>(x)) had been called) is, as I see it, that they can be used to simplify writing templates. So, those overloads do something meaningful in that respect. :-)

[BillyONeal]

@BillyONeal The usefulness of std::is_integral overloads for std::isnan etc. (which I assume will just return false as-if return isnan(static_cast<double>(x)) had been called) is, as I see it, that they can be used to simplify writing templates. So, those overloads do something meaningful in that respect. :-)

That is actually want scares me: it encourages code that looks correct but falls over if the user passes more than 53 bits; I would kinda like to see the user explicitly if constexpr in those places to indicate that they have considered both archetypical arithmetic types.

[TedLyngmo]

but falls over if the user passes more than 53 bits

Are you thinking about the functions that'll return a double? If so, yes, I think it's was a bad idea to add integral type overloads for those. For the functions returning bool (not only signbit) I think the overloads have real value.

I would kinda like to see the user explicitly if constexpr in those places to indicate that they have considered both archetypical arithmetic types.

isnan(value) in an instantiation of a template where value is an integral type will (most probably, or even should) be optimized to (false) and then that branch will be discarded - but if we're talking about functions returning double I totally agree.

[BillyONeal]

Are you thinking about the functions that'll return a double?

I mean all code that might accidentally mix these things. If the user called something in the fpclassify family that suggests that they are expecting the inputs to behave like floats. Consider the straw man:

template<class T>
T f(T x) {
    T y = x + 42.0;
    if (isfinite(y)) { y = -y; }
    return y;
}

works great until T is long long and the input doesn't fit in 53 bits. Maybe stopping that with this isn't a huge win, but I think the amount of code that becomes truly generic across floats and integrals but for these functions is small. But I also am probably not the target user here; that's why I wanted the committee to provide clarification here.

[TedLyngmo]

that's why I wanted the committee to provide clarification here

Great! Have you or somebody else written a proposal? I'd like to see where this goes ....

[statementreply]

Examples where the loss of precision when converted to double does change the result:

const long long x = 0x0fff'ffff'ffff'ffffLL;
assert(ilogb(x) == 59); // fails
assert(llround(x) == x); // fails

[BillyONeal]

Just got pinged internally about this one again today :(.

[MooglyGuy]

Is there ever going to be movement on this? As it is, calling std::signbit on an int32_t is, according to MSVC 2022 17.4.4, "ambiguous". It seems pretty unambiguous to me, but I am not MSVC 2022.

What the language WG says aside, this is one of a handful of ongoing issues where MSVC stands apart from pretty much every other major compiler in terms of being able to compile a large project like MAME. Is ideology more important than a working compiler?

[BillyONeal]

@MooglyGuy the reason we didn't just "fix" this is it is not clear that that is a "fix" or "working". Passing integers to the fpclassify family looks like a bug. I wanted confirmation from the people who actually own the answer as to whether that is intended to work, or intended to be diagnosed as the semantic error that it is. Permanent in source workarounds are easy: cast to double first or guard it for is_floating_point.

But I am no longer a maintainer, perhaps a current one has a different read on the situation.

[CaseyCarter]

WG21-P1467 significantly rewrote all of the wording in [cmath.syn]. It is now clear and unambiguous that the floating-point classification functions, including signbit, should have so-called "sufficient additional overloads". The paper originated in SG6 (WG21's numerics study group) so the wording is the product of domain experts which was then approved by LEWG, LWG, and WG21 entire in plenary. I don't think we can get any more clear direction than that.

[MooglyGuy]

My main concern here doesn't even involve floating-point types, std::signbit can purportedly be applied to integral types - in which case it would presumably return the most significant bit of the binary representation of that type.

That's the origin of my frustration: I can completely understand a level of faff involving the function's implementation for floating-point types due to the fact that IEEE 754 was never a particularly well-defined spec to begin with. But I would expect the function's implementation for integers to be a non-issue as a result, not for the compiler to be popping ambiguity errors.

[frederick-vs-ja]

I think this issue could be partially fixed by inserting the follow code snippet into <cstdlib>, immediately before #include <math.h>. A complete resoultion needs changes in UCRT.

// #if _STL_COMPILER_PREPROCESSOR

#include <xtr1common>

template <class _Ty, _STD enable_if_t<_STD is_integral_v<_Ty>, int> = 0>
_NODISCARD _Check_return_ _CONSTEXPR23 int fpclassify(_In_ const _Ty _Ix) noexcept /* strengthened */ {
    return _Ix ? -2 : 0; // FP_NORMAL and FP_ZERO in UCRT respectively
}

template <class _Ty, _STD enable_if_t<_STD is_integral_v<_Ty>, int> = 0>
_NODISCARD _Check_return_ _CONSTEXPR23 bool signbit(_In_ const _Ty _Ix) noexcept /* strengthened */ {
    if constexpr (static_cast<_Ty>(-1) < _Ty{}) {
        return _Ix < 0;
    } else {
        return false;
    }
}

// #include <math.h>

After such change, if one includes <cstdlib> or <cmath> before <math.h> or just doesn't use <math.h> directly, then these overloads behaves correctly.

However, if one just includes <math.h> or includes it before <cstdlib>/<cmath>, then this issue still exists.

[BillyONeal]

WG21-P1467 significantly rewrote all of the wording in [cmath.syn]. It is now clear and unambiguous that the floating-point classification functions, including signbit, should have so-called "sufficient additional overloads". The paper originated in SG6 (Wg21's numerics study group) so the wording is the product of domain experts which was then approved by LEWG, LWG, and WG21 entire in plenary. I don't think we can get any more clear direction than that.

Thanks for the update! Consider my objection to going here withdrawn.

My main concern here doesn't even involve floating-point types, std::signbit can purportedly be applied to integral types - in which case it would presumably return the most significant bit of the binary representation of that type.

That's the origin of my frustration: I can completely understand a level of faff involving the function's implementation for floating-point types due to the fact that IEEE 754 was never a particularly well-defined spec to begin with. But I would expect the function's implementation for integers to be a non-issue as a result, not for the compiler to be popping ambiguity errors.

Again, the issue was never "is this complicated to implement", it was "this function's effects, by virtue of being part of the fpclassify family, really only make sense when applied to floating point, and any attempt to apply them to integers is likely an error". After all, for integers it is equivalent to < 0. The only reason to call this thing is to handle negative zeros, or possibly some NAN/infinity edge cases, which aren't a thing for integers.

[MooglyGuy]

Again, the issue was never "is this complicated to implement", it was "this function's effects, by virtue of being part of the fpclassify family, really only make sense when applied to floating point, and any attempt to apply them to integers is likely an error". After all, for integers it is equivalent to < 0. The only reason to call this thing is to handle negative zeros, or possibly some NAN/infinity edge cases, which aren't a thing for integers.

std::signbit also adds legibility for cases when you're looking for an integer value crossing zero. That said, after spending some time to look at the spec with a fresh pair of eyes, I suspect you're correct that the use case I'm encountering is more of an abuse of the spec than anything.

It should indeed compile, but you're probably more correct that the affected code should just be using std::signbit.

[StephanTLavavej]

I'm going to resolve this as fixed by #4537, since this now works with <cmath> and there's nothing left for the STL to do here. DevCom-10294165 tracks the UCRT's issue with plain <math.h>.