Skip to content

std.math: Move most of the rounding functions into submodule.#7943

Merged
dlang-bot merged 1 commit intodlang:masterfrom
berni44:math_move_rounding
Apr 12, 2021
Merged

std.math: Move most of the rounding functions into submodule.#7943
dlang-bot merged 1 commit intodlang:masterfrom
berni44:math_move_rounding

Conversation

@berni44
Copy link
Contributor

@berni44 berni44 commented Apr 7, 2021

I left out floor and ceil because moving floorImpl to the submodule fails with a strange error message. I guess, there is some compiler magic involved here, making union work in CTFE.

@berni44 berni44 requested a review from ibuclaw as a code owner April 7, 2021 13:13
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @berni44! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#7943"

@berni44 berni44 force-pushed the math_move_rounding branch from 57e32df to e7d7974 Compare April 7, 2021 13:18
@berni44
Copy link
Contributor Author

berni44 commented Apr 7, 2021

Here compilable/test5227.d fails. It's two function that cannot be evaluated at runtime: round and trunc. Looks, like the calls to core.stdc.math.roundl and core.stdc.math.truncl in CTFE are restricted to std.math, but cannot be called from std.math.rounding.

@berni44
Copy link
Contributor Author

berni44 commented Apr 7, 2021

It's not the calls I mentioned above. So I suspect dmd searching round and trunc in std.math and not in std.math.rounding; that is, the public imports are ignored... I need help here from someone who knows the inner workings of dmd.

@thewilsonator
Copy link
Contributor

@RazvanN7
Copy link
Collaborator

RazvanN7 commented Apr 8, 2021

Have you tried making a public alias to the symbols that were moved to the submodules? That should work.

Otherwise, we can make the compiler acknowledge the new submodules, although I would rather not go down that route.

@berni44
Copy link
Contributor Author

berni44 commented Apr 8, 2021

Have you tried making a public alias to the symbols that were moved to the submodules? That should work.

Wouldn't that clash with the public imports? I'll check it out.

@berni44
Copy link
Contributor Author

berni44 commented Apr 8, 2021

Didn't clash, so I probably do not understand something with visibility and scope... But that's my fault. I'll try to pick that up.

Anyway, the error has not vanished, but this time brought up a new detail:

D:\a\1\dmd\test\..\..\phobos\std\math\package.d(4851): Error: `asm` statements cannot be interpreted at compile time
D:\a\1\dmd\test\..\..\phobos\std\math\rounding.d(437):        called from here: `getControlState()`
compilable\test5227.d(70):        called from here: `round(5.5L)`
compilable\test5227.d(70):        while evaluating `pragma(msg, roundr)`

Line 4851 of package.d is inside getControlState() of FloatingPointControl.

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

Squash and add missing versions.

@berni44 berni44 force-pushed the math_move_rounding branch from d00bc65 to 0c96600 Compare April 11, 2021 06:54
@dlang-bot dlang-bot merged commit b1f21e8 into dlang:master Apr 12, 2021
@berni44 berni44 deleted the math_move_rounding branch April 28, 2021 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants