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.

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

Fix Issue 20618 - std.random.unpredictableSeed can work in betterC #7408

Closed
wants to merge 1 commit into from

Conversation

n8sh
Copy link
Member

@n8sh n8sh commented Feb 27, 2020

The diff is easier to review with whitespace changes hidden.

@n8sh n8sh added the BetterC label Feb 27, 2020
@n8sh n8sh requested a review from wilzbach as a code owner February 27, 2020 12:26
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close Bugzilla Severity Description
20618 enhancement std.random.unpredictableSeed can work in betterC

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

@n8sh
Copy link
Member Author

n8sh commented Feb 27, 2020

At least on my machine the @betterC test extractor doesn't seem to be running any tests (it always succeeds). I manually tested this locally but this probably shouldn't be merged unless or until automatic tests are working properly.

@@ -1656,7 +1657,7 @@ version (AnyARC4Random)
}
else
{
private ulong bootstrapSeed() @nogc nothrow
private ulong bootstrapSeed()() @nogc nothrow
Copy link
Member Author

Choose a reason for hiding this comment

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

On my machine changing this function to a template was necessary to avoid a symbol not found error with betterC. This seems like a bug.

Copy link
Contributor

@kinke kinke Mar 12, 2020

Choose a reason for hiding this comment

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

Why would that be a bug? Everything that works in betterC must be templated (or inlined), as druntime and Phobos aren't linked. So of course we could needlessly templatize most of druntime, but what would be the point? Static functionality that can be precompiled once for many compilation cycles does provide benefits.

Copy link
Member Author

Choose a reason for hiding this comment

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

I misunderstood how betterC treats Phobos then. I thought it treated it like any library.

@wilzbach wilzbach mentioned this pull request Feb 27, 2020
@wilzbach
Copy link
Member

At least on my machine the @betterC test extractor doesn't seem to be running any tests (it always succeeds). I manually tested this locally but this probably shouldn't be merged unless or until automatic tests are working properly.

Good catch! -> #7409

@n8sh
Copy link
Member Author

n8sh commented Mar 17, 2020

#7408 (comment)

So of course we could needlessly templatize most of druntime, but what would be the point? Static functionality that can be precompiled once for many compilation cycles does provide benefits.

Closing because I now have doubts about whether this PR is a net improvement.

@n8sh n8sh closed this Mar 17, 2020
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.

4 participants