Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update math.algebraic.d#abs to return correct integral type #8861

Merged
merged 6 commits into from
Dec 15, 2023
Merged

Update math.algebraic.d#abs to return correct integral type #8861

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9073c1d
Change return type of abs to conform with documentation
HuskyNator Dec 10, 2023
473bb75
Fix tests
HuskyNator Dec 10, 2023
ba8a578
Merge branch 'abs'
HuskyNator Dec 10, 2023
00a2b65
Separate assert statements
HuskyNator Dec 12, 2023
e425dc6
Document problematic behaviour
HuskyNator Dec 15, 2023
e5e6f60
Fix misplaced doc backticks and reorder attributes
HuskyNator Dec 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions std/math/algebraic.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,24 +43,23 @@ import std.traits : CommonType, isFloatingPoint, isIntegral, isSigned, Unqual;
* the return type will be the same as the input.
*
* Limitations:
* Does not work correctly for signed intergal types and value `Num`.min.
* Does not work correctly for value `Num`.min.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't simply remove "signed intergal types and" because it does work correctly for Num.min of unsigned or float types. It is worded poorly though.

Suggested change
* Does not work correctly for value `Num`.min.
* Does not work correctly for value `Num`.min of signed integral types because of overflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that makes sense.
Would it be proper to explicitly defined this as undefined behaviour as well?
This seems reasonable to me as described below (#8861 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't be undefined behavior without the function becoming @system, and we want to keep abs @safe. You can make it unspecified behavior, but I don't see what's wrong with just telling it overflows and stays the same. Existing code might be checking for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should also say more clearly what it actually does rather than just "doesn't work correctly" — in fact, D has defined semantics here so is it wrong?

Copy link
Contributor Author

@HuskyNator HuskyNator Dec 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what behaviour of abs you expect in this case.
"Calculates the absolute value of a number" is not representable in the case of int.min. From where I'm standing this is currently undefined. Do we expect saturation, zero, identity? (int.max / 0 / int.min)
Do we even want to define behaviour for abs(Num.min) at all?

Under the current code the return value of Num.min, in the case of signed integral types, will be itself (identity), as -Num.min will return itself (overflow). I view this more as an implementation side-effect.

I don't believe this should be defined behaviour.
As an example, this is what cpp's (int/long/long long) std::abs states:

The behavior is undefined if the result cannot be represented by the return type.

On a side-tangent, I've searched for the specifications of unary - before, but I believe these have been implicitly borrowed from C(++). I've been unable to locate them in the documentation. This goes for both -Num.min and -unsigned: https://discord.com/channels/242094594181955585/242122752436338688/1169211488062406686 .
This should definitely be explicitly defined behaviour.

*/
auto abs(Num)(Num x) @nogc pure nothrow
if ((is(immutable Num == immutable short) || is(immutable Num == immutable byte)) ||
(is(typeof(Num.init >= 0)) && is(typeof(-Num.init))))
if (isIntegral!Num || (is(typeof(Num.init >= 0)) && is(typeof(-Num.init))))
{
static if (isFloatingPoint!(Num))
return fabs(x);
else
{
static if (is(immutable Num == immutable short) || is(immutable Num == immutable byte))
return x >= 0 ? x : cast(Num) -int(x);
static if (isIntegral!Num)
return x >= 0 ? x : cast(Num) -x;
else
return x >= 0 ? x : -x;
}
}

/// ditto
///
@safe pure nothrow @nogc unittest
{
import std.math.traits : isIdentical, isNaN;
Expand All @@ -70,16 +69,19 @@ if ((is(immutable Num == immutable short) || is(immutable Num == immutable byte)
assert(abs(-real.infinity) == real.infinity);
assert(abs(-56) == 56);
assert(abs(2321312L) == 2321312L);
assert(abs(23u) == 23u);
}

@safe pure nothrow @nogc unittest
{
short s = -8;
byte b = -8;
assert(abs(s) == 8);
assert(abs(b) == 8);
immutable(byte) c = -8;
assert(abs(c) == 8);
assert(abs(byte(-8)) == 8 && is(typeof(abs(byte(-8))) == byte));
HuskyNator marked this conversation as resolved.
Show resolved Hide resolved
assert(abs(ubyte(8u)) == 8 && is(typeof(abs(ubyte(8u))) == ubyte));
assert(abs(short(-8)) == 8 && is(typeof(abs(short(-8))) == short));
assert(abs(ushort(8u)) == 8 && is(typeof(abs(ushort(8u))) == ushort));
assert(abs(int(-8)) == 8 && is(typeof(abs(int(-8))) == int));
assert(abs(uint(8u)) == 8 && is(typeof(abs(uint(8u))) == uint));
assert(abs(long(-8)) == 8 && is(typeof(abs(long(-8))) == long));
assert(abs(ulong(8u)) == 8 && is(typeof(abs(ulong(8u))) == ulong));
}

@safe pure nothrow @nogc unittest
Expand Down