Skip to content

Comments

Annotate std/bigint.d and std/internal/math/biguintcore.d to please d…#8081

Merged
WalterBright merged 1 commit intodlang:masterfrom
nordlow:fix-pure-scope-bigint
May 18, 2021
Merged

Annotate std/bigint.d and std/internal/math/biguintcore.d to please d…#8081
WalterBright merged 1 commit intodlang:masterfrom
nordlow:fix-pure-scope-bigint

Conversation

@nordlow
Copy link
Contributor

@nordlow nordlow commented May 16, 2021

…lang/dmd#12520

Part of #8076

@dlang-bot
Copy link
Contributor

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

std/bigint.d Outdated
{
BigInt r = this;
return r.opOpAssign!(op)(y);
r.opOpAssign!(op)(y);
Copy link
Contributor Author

@nordlow nordlow May 16, 2021

Choose a reason for hiding this comment

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

I'm gonna see if I can get this to build without this change.

@nordlow nordlow changed the title Annotate std/bigint.d and std/internal/math/biguintcore.d to please d… WIP: Annotate std/bigint.d and std/internal/math/biguintcore.d to please d… May 16, 2021
@nordlow
Copy link
Contributor Author

nordlow commented May 18, 2021

Is

    static void divMod(scope return BigUint x, scope BigUint y,
                       scope out BigUint quotient, scope out BigUint remainder) pure nothrow @safe
    {
        if (y.data.length > x.data.length)
        {
            quotient = 0uL;
            remainder = x;
        }
        else if (y.data.length == 1)
        {
            quotient = divInt(x, y.data[0]);
            remainder = BigUint([modInt(x, y.data[0])]);
        }
        else
        {
            BigDigit[] quot = new BigDigit[x.data.length - y.data.length + 1];
            BigDigit[] rem = new BigDigit[y.data.length];
            divModInternal(quot, rem, x.data, y.data);
            quotient = BigUint(removeLeadingZeros(trustedAssumeUnique(quot)));
            remainder = BigUint(removeLeadingZeros(trustedAssumeUnique(rem)));
        }
    }

the correct parameter qualification of x and y, @Geod24, @thewilsonator, @WalterBright? Or does the return qualifier of x only apply if it's returned in a normal return?

I'm asking because I can't find any references on assigning scope (potentially return) parameter to scope out parameters in the DIP-25 and DIP-1000 specs.

@thewilsonator
Copy link
Contributor

As far as I understand:
return qualified parameters on a void returning function are nonsensical.
out parameters don't make sense as scope in a pure function since they can only be derived from either a pure allocator (e.g. the GC) or from other input parameters. scope on function parameters means that that reference shall not escape the scope of the function.

@nordlow
Copy link
Contributor Author

nordlow commented May 18, 2021

As far as I understand:
return qualified parameters on a void returning function are nonsensical.
out parameters don't make sense as scope in a pure function since they can only be derived from either a pure allocator (e.g. the GC) or from other input parameters. scope on function parameters means that that reference shall not escape the scope of the function.

Ok, thanks. Adjusted to

    static void divMod(scope BigUint x, scope BigUint y,
                       out BigUint quotient, out BigUint remainder) pure nothrow @safe

but that fails as

std/internal/math/biguintcore.d(977): Error: scope variable `x` assigned to `remainder` with longer lifetime
std/internal/math/biguintcore.d(981): Error: scope variable `x` assigned to `quotient` with longer lifetime
std/internal/math/biguintcore.d(977): Error: scope variable `x` assigned to `remainder` with longer lifetime
std/internal/math/biguintcore.d(981): Error: scope variable `x` assigned to `quotient` with longer lifetime

Qualifying quotient and remainder as scope has no effect.

What to do, @WalterBright @thewilsonator ?

@dkorpel
Copy link
Contributor

dkorpel commented May 18, 2021

Why does divMod need scope parameters, is there any existing code calling it with scope BigInts?
If you have to make it work, you can do:

// biguintcore.d(977):
remainder = BigUint(x.data.idup);
// biguintcore.d(981):
quotient = divInt(BigUint(x.data.idup), y.data[0]);

If you do that, maybe add a comment that the redundant allocation is due to the limitation that return scope for out parameters is not supported.

@nordlow
Copy link
Contributor Author

nordlow commented May 18, 2021

Why does divMod need scope parameters, is there any existing code calling it with scope BigInts?
If you have to make it work, you can do:

// biguintcore.d(977):
remainder = BigUint(x.data.idup);
// biguintcore.d(981):
quotient = divInt(BigUint(x.data.idup), y.data[0]);

If you do that, maybe add a comment that the redundant allocation is due to the limitation that return scope for out parameters is not supported.

I'm surprised this limitation hasn't been brought up. Has return scope for out parameters been considered, @WalterBright and @andralex ?

@nordlow
Copy link
Contributor Author

nordlow commented May 18, 2021

Why does divMod need scope parameters, is there any existing code calling it with scope BigInts?
If you have to make it work, you can do:

Thanks for highlighting this. I've reverted to

static void divMod(BigUint x, scope BigUint y,
                       out BigUint quotient, out BigUint remainder) pure nothrow @safe

that doesn't require any @trusted and that passes locally with dmd master my machine. Parameter x indeed doesn't need scope qualifier.

I also made

BigUint.this(immutable(BigDigit) [] x)

@safe since it need to be anymore.

So no more @trusted remaining! Yay!

make -f posix.mak unittest passes locally on my machine.

Made title non-WIP.

Ready now, @RazvanN7 and @thewilsonator.

@nordlow nordlow force-pushed the fix-pure-scope-bigint branch from 5e12829 to b699ca9 Compare May 18, 2021 11:09
@nordlow nordlow changed the title WIP: Annotate std/bigint.d and std/internal/math/biguintcore.d to please d… Annotate std/bigint.d and std/internal/math/biguintcore.d to please d… May 18, 2021
@nordlow nordlow force-pushed the fix-pure-scope-bigint branch from b699ca9 to a44f718 Compare May 18, 2021 11:14
@WalterBright WalterBright merged commit f0e8bcf into dlang:master May 18, 2021
@nordlow nordlow deleted the fix-pure-scope-bigint branch October 19, 2021 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants