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

HuskyNator · 2023-12-10T18:13:58Z

The documentation states the return type for an argument of integral types (assuming under phobos: (u)byte,short,int,long) should equal its own type.

Under current implementation this is not the case. Exemptions are made for short and byte.
Using isIntegral instead yields the correct behavior, also for unsigned types (which simply return themselves).

Integral types should simply return their own type.

dlang-bot · 2023-12-10T18:14:02Z

Thanks for your pull request and interest in making D better, @HuskyNator! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	24278	minor	std.math.abs promotes unsigned argument to 32 bits

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8861"

dkorpel

I think the weird treatment of byte and short is a remnant from the transition to new integer promotion rules of unary minus, but since that transition is over, it's good to remove that code.

This needs a changelog entry or bugzilla issue.

dkorpel · 2023-12-10T18:47:16Z

std/math/algebraic.d

@@ -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.


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.

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

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.

std/math/algebraic.d

maxhaton · 2023-12-10T19:04:19Z

std/math/algebraic.d

@@ -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.


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?

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.

HuskyNator · 2023-12-12T15:37:14Z

I think the weird treatment of byte and short is a remnant from the transition to new integer promotion rules of unary minus, but since that transition is over, it's good to remove that code.

This needs a changelog entry or bugzilla issue.

I'm not sure how or where to add this (and have difficulty accessing bugzilla at all).
If truly necessary, I'd gladly have some help 😅.

dkorpel · 2023-12-12T15:44:06Z

I filed an issue: https://issues.dlang.org/show_bug.cgi?id=24278
If you include "Fix 24278" or "Fix issue 24278" in one of the commit messages, the bot will auto-close the issue and add it to the change log.

Reorder attributes as per style guidelines: > Attributes should be listed in alphabetical ordering Fix issue 24278

HuskyNator · 2023-12-15T03:21:08Z

I'm not sure why 2 tests fail. Their error message suggests a different issue.

thewilsonator · 2023-12-15T04:39:24Z

I'm not sure why 2 tests fail. Their error message suggests a different issue.

cc @kinke is this caused by your recent makefile changes?

kinke · 2023-12-15T14:58:51Z

is this caused by your recent makefile changes?

Yes; fix here: dlang/dmd#15911

HuskyNator added 3 commits December 10, 2023 16:00

Change return type of abs to conform with documentation

9073c1d

Integral types should simply return their own type.

Fix tests

473bb75

Merge branch 'abs'

ba8a578

HuskyNator requested review from ibuclaw and dnadlinger as code owners December 10, 2023 18:13

dkorpel reviewed Dec 10, 2023

View reviewed changes

maxhaton reviewed Dec 10, 2023

View reviewed changes

Separate assert statements

00a2b65

HuskyNator added 2 commits December 15, 2023 01:32

Document problematic behaviour

e425dc6

Fix misplaced doc backticks and reorder attributes

e5e6f60

Reorder attributes as per style guidelines: > Attributes should be listed in alphabetical ordering Fix issue 24278

dlang-bot added the Severity:Bug Fix label Dec 15, 2023

dkorpel approved these changes Dec 15, 2023

View reviewed changes

dkorpel merged commit 0612f55 into dlang:master Dec 15, 2023
12 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

HuskyNator commented Dec 10, 2023

dlang-bot commented Dec 10, 2023 •

edited

Loading

dkorpel left a comment

dkorpel Dec 10, 2023

HuskyNator Dec 12, 2023

dkorpel Dec 12, 2023

maxhaton Dec 10, 2023

HuskyNator Dec 12, 2023 •

edited

Loading

HuskyNator commented Dec 12, 2023

dkorpel commented Dec 12, 2023

HuskyNator commented Dec 15, 2023

thewilsonator commented Dec 15, 2023

kinke commented Dec 15, 2023

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

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

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

Conversation

HuskyNator commented Dec 10, 2023

dlang-bot commented Dec 10, 2023 • edited Loading

Bugzilla references

Testing this PR locally

dkorpel left a comment

Choose a reason for hiding this comment

dkorpel Dec 10, 2023

Choose a reason for hiding this comment

HuskyNator Dec 12, 2023

Choose a reason for hiding this comment

dkorpel Dec 12, 2023

Choose a reason for hiding this comment

maxhaton Dec 10, 2023

Choose a reason for hiding this comment

HuskyNator Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

HuskyNator commented Dec 12, 2023

dkorpel commented Dec 12, 2023

HuskyNator commented Dec 15, 2023

thewilsonator commented Dec 15, 2023

kinke commented Dec 15, 2023

dlang-bot commented Dec 10, 2023 •

edited

Loading

HuskyNator Dec 12, 2023 •

edited

Loading