Skip to content

fix covariant behavior with 'return scope'#6429

Merged
WalterBright merged 1 commit intodlang:masterfrom
WalterBright:scope-covariant
Jan 15, 2017
Merged

fix covariant behavior with 'return scope'#6429
WalterBright merged 1 commit intodlang:masterfrom
WalterBright:scope-covariant

Conversation

@WalterBright
Copy link
Member

@WalterBright WalterBright commented Jan 11, 2017

This is a significantly better way to check for covariance of function parameters that have combinations of ref, return and scope. It's reduced to an enumeration of 8 cases in enum SR which then gets fed into a matrix covariant[][] to produce a yay or nay on covariance.

It makes it feasible to reason about this without the various bits of it scattered all through the code.

src/mtype.d Outdated
static uint combine(uint from, uint to) pure nothrow @nogc @safe
{
return (from << 2) | to;
}
Copy link
Member

Choose a reason for hiding this comment

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

You could move the translation into the combine instead of introducing another enum.
combine(STCreturn | STCscope, 0)
The cases are CTFE'ed anyhow.

return (from << 2) | to;
}

/* result is true if the 'from' can be used as a 'to'
Copy link
Member

Choose a reason for hiding this comment

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

The useful we can add scope, but not subtract it, we can subtract return but not add it comments got lost.

@mathias-lang-sociomantic
Copy link
Contributor

What is this fixing ? There's neither description nor a test case.

@WalterBright
Copy link
Member Author

There's neither description nor a test case.

That's why I marked it as "needs work". I'm working on finding a way to resolve an ambiguity I discovered, and needed to know what the existing test suite thought of my changes.

@WalterBright WalterBright force-pushed the scope-covariant branch 5 times, most recently from 28753f1 to 6916abc Compare January 14, 2017 05:33
@WalterBright
Copy link
Member Author

@MartinNowak what's going on with the cycle detection in druntime?

@WalterBright
Copy link
Member Author

Blocking dlang/druntime#1733

@WalterBright WalterBright added Review:Blocking Other Work review and pulling should be a priority and removed Review:Needs Work labels Jan 14, 2017
@WalterBright
Copy link
Member Author

With this, druntime now compiles without changes with -dip1000 yay!

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

Please improve coverage

* returnByRef = true if the function returns by ref
* stc = storage class of parameter
*/
static uint buildSR(bool returnByRef, StorageClass stc) pure nothrow @nogc @safe
Copy link
Member

Choose a reason for hiding this comment

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

this function seems uncovered

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems the coverage data is messed up - the enums further on are not executable code.

covariant[SR.Ref ][SR.ReturnRef] = true;
covariant[SR.ReturnRef_Scope][SR.ReturnRef] = true;
covariant[SR.Ref_ReturnScope][SR.Ref ] = true;
covariant[SR.Ref_ReturnScope][SR.ReturnRef] = true;
Copy link
Member

Choose a reason for hiding this comment

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

You're introducing a constructor cycles here.

./dmd/src/dmd -c -o- main --DRT-oncycle=print                                                                         20:19:50
Cyclic dependency between module ddmd.traits and ddmd.mtype
ddmd.traits* ->
ddmd.mtype* ->
ddmd.expression ->
ddmd.traits*

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to run this in CTFE instead of in a constructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I thought the error came from running one of the test programs, not the compiler itself. But there's nothing platform specific there - why is the error happening only one some platforms?

covariant[SR.Ref ][SR.ReturnRef] = true;
covariant[SR.ReturnRef_Scope][SR.ReturnRef] = true;
covariant[SR.Ref_ReturnScope][SR.Ref ] = true;
covariant[SR.Ref_ReturnScope][SR.ReturnRef] = true;
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to run this in CTFE instead of in a constructor.

@WalterBright
Copy link
Member Author

You should be able to run this in CTFE instead of in a constructor.

Good idea. Done.

@WalterBright WalterBright merged commit 9b26093 into dlang:master Jan 15, 2017
@WalterBright WalterBright deleted the scope-covariant branch January 15, 2017 02:41
@MartinNowak
Copy link
Member

Always confuses me that Parameter.isCovariant is used when checking for contravariant parameters.

@WalterBright
Copy link
Member Author

I never remember which is which, either.

@MartinNowak
Copy link
Member

As it's an ordering relation (e.g. subtype) the same comparison operator can be used for both, just that covariant checks A <= B and contravariant A >= B (or vice versa 😉).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review:Blocking Other Work review and pulling should be a priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants