Skip to content

Comments

biguintcore.d: add return scope annotations#8105

Closed
WalterBright wants to merge 1 commit intodlang:masterfrom
WalterBright:biguintcore-ret
Closed

biguintcore.d: add return scope annotations#8105
WalterBright wants to merge 1 commit intodlang:masterfrom
WalterBright:biguintcore-ret

Conversation

@WalterBright
Copy link
Member

These are the changes from #8076 made to std.internal.math.biguintcore to keep the PR size down. It adds return and scope annotations.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

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

@WalterBright
Copy link
Member Author

blocking #8104

@dkorpel
Copy link
Contributor

dkorpel commented May 18, 2021

Have you seen @nordlow 's WIP PR, #8081 , and have you seen that there's still three @trusted TODO's in here?

@WalterBright
Copy link
Member Author

No, I didn't know #8081 existed. This one, however, passes and it will reduce the scope (!) of the remaining issues in #8081 so it can concentrate on getting it to pass.

I'm a big fan of incremental improvements!

@WalterBright
Copy link
Member Author

Getting this merged will also enable #8104 which will further reduce the scope of the remaining work to be done.

@WalterBright
Copy link
Member Author

Note that this PR is an exact copy of @nordlow 's work on biguintcore.d, it is not my work.

@dkorpel
Copy link
Contributor

dkorpel commented May 18, 2021

Note that this PR is an exact copy of @nordlow 's work on biguintcore.d, it is not my work.

It's a copy of his WIP work, it's still being worked on in #8081

@dkorpel
Copy link
Contributor

dkorpel commented May 18, 2021

No, I didn't know #8081 existed. This one, however, passes and it will reduce the scope (!) of the remaining issues in #8081 so it can concentrate on getting it to pass.

My concern is that when we rush this, we move from "memory corruption because of dmd bug" to "memory corruption because of overzealous @trusted in Phobos". A TODO comment can easily linger for years, it will only get fixed once an unfortunate programmer encounters memory corruption in his @safe BigInt code.

@nordlow
Copy link
Contributor

nordlow commented May 18, 2021

My concern is that when we rush this, we move from "memory corruption because of dmd bug" to "memory corruption because of overzealous @trusted in Phobos". A TODO comment can easily linger for years, it will only get fixed once an unfortunate programmer encounters memory corruption in his @safe BigInt code.

My intention is to remove them as soon as the dmd branch has been merged. Shall I make a list of them in the dmd PR?

I've added references to the phobos PRs at dlang/dmd#12520 that use @trusted to elide faulty return diagnostics.

@WalterBright
Copy link
Member Author

@nordlow I appreciate the work you're doing here. My goal here is to:

  1. reduce the size of PRs
  2. get the low-hanging fruit merged so it won't be delayed by work on the hard stuff
  3. get the "leaf" modules working, so other modules that depend on them can be worked on in parallel.

(biguintcore appears to be a leaf file!)

I defer to you on this, @nordlow !

@dkorpel
Copy link
Contributor

dkorpel commented May 18, 2021

My goal here is to:
1. reduce the size of PRs

@nordlow has already been doing that, see: #8076 (comment)
GitHub shows referenced pull requests above that comment, most of them have been merged already!

@nordlow
Copy link
Contributor

nordlow commented May 18, 2021

Please consider merging #8081 instead. I made to compile locally without any need for extra @trusted.

@WalterBright
Copy link
Member Author

Closed in favor of #8081

@WalterBright WalterBright deleted the biguintcore-ret branch May 18, 2021 16:41
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.

6 participants