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 20053 - add mixin types #10215

Merged
merged 1 commit into from
Jul 29, 2019
Merged

Conversation

WalterBright
Copy link
Member

Currently D has these mixins:

  1. declaration
  2. statement
  3. expression

But lacks types. This adds mixin types. I.e. adding:

 MixinType:
    mixin ( ArgumentList)

to https://dlang.org/spec/declaration.html#BasicType

@WalterBright WalterBright requested a review from RazvanN7 as a code owner July 24, 2019 10:13
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
20053 enhancement add mixin types

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#10215"

@WalterBright
Copy link
Member Author

@TurkeyMan FYI

@thewilsonator
Copy link
Contributor

home/circleci/dmd/src/dmd/typesem.d(1893): Error: template instance Parser!ASTCodegen template Parser is not defined

@thewilsonator thewilsonator added the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Jul 24, 2019
@TurkeyMan
Copy link
Contributor

Thank you so much. This is really good.

@WalterBright
Copy link
Member Author

Yah, it was a rather glaring gap in the mixin capability.

@WalterBright WalterBright force-pushed the mixinType branch 2 times, most recently from 1434478 to cbbcfc1 Compare July 24, 2019 10:22
@WalterBright
Copy link
Member Author

I'll try to get it through the test suite tomorrow.

@Geod24
Copy link
Member

Geod24 commented Jul 25, 2019

Please add the following to a fail_compilation test:

struct Foo {
    alias T = mixin("T2");
}
alias T1 = mixin("Foo.T");
alias T2 = mixin("T1");
void func (T2 p) {}

Or any similar test with circular reference

@TurkeyMan
Copy link
Contributor

@Geod24 Nice thinking :)

@WalterBright
Copy link
Member Author

Spec: dlang/dlang.org#2684

@WalterBright WalterBright removed the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Jul 25, 2019
@WalterBright WalterBright force-pushed the mixinType branch 2 times, most recently from e385c19 to 3cd3c22 Compare July 25, 2019 09:15
return a;
}

return new TypeMixin(arraySyntaxCopy(exps));
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused as to why this idiom was used here. It's just a more complicated way of saying... this:

override Type syntaxCopy()
{
    Expressions* a = null;
    if (exps)
    {
        a = new Expressions(exps.dim);
        foreach (i, e; *exps)
        {
            (*a)[i] = e ? e.syntaxCopy() : null;
        }
    }
    return new TypeMixin(a);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Because arraySyntaxCopy is used all over dmd, but is not available in astbase.d

src/dmd/parse.d Outdated
*/
RootObject parseTypeOrAssignExp()
{
return (isDeclaration(&token, NeedDeclaratorId.no, TOK.reserved, null))
Copy link
Member

Choose a reason for hiding this comment

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

superfluous parens

Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

Please also add a test for multiple arguments to mixin (e.g. mixin("a", ".", "b");)

@@ -6413,6 +6413,7 @@ extern (C++) class TemplateInstance : ScopeDsymbol
if (ta)
{
//printf("type %s\n", ta.toChars());

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated

{
return isDeclaration(&token, NeedDeclaratorId.no, TOK.reserved, null)
? parseType() // argument is a type
: parseAssignExp(); // argument is an expression
Copy link
Member

Choose a reason for hiding this comment

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

Would have been better if this refactoring was in another PR

// https://dlang.org/spec/expression.html#mixin_types
nextToken();
if (token.value != TOK.leftParentheses)
error("found `%s` when expecting `%s` following %s", token.toChars(), Token.toChars(TOK.leftParentheses), "`mixin`".ptr);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
error("found `%s` when expecting `%s` following %s", token.toChars(), Token.toChars(TOK.leftParentheses), "`mixin`".ptr);
error("found `%s` when expecting `%s` following `mixin`", token.toChars(), Token.toChars(TOK.leftParentheses));

@atilaneves atilaneves merged commit 7900a3d into dlang:master Jul 29, 2019
@ghost
Copy link

ghost commented Jul 30, 2019

This has been merged without

  • changelog entry
  • h file counterpart
  • dlang.org PR for this.

Same standards for everyone please.

@WalterBright WalterBright deleted the mixinType branch July 30, 2019 20:23
@WalterBright
Copy link
Member Author

h file counterpart

Don't need it.

dlang.org PR for this.

#10215 (comment)

@WalterBright
Copy link
Member Author

Please also add a test for multiple arguments to mixin (e.g. mixin("a", ".", "b");)

Not necessary due to common handling with other mixins. It's only necessary to test what is different.

@ghost
Copy link

ghost commented Jul 30, 2019

changelog PR and we're good. Something inside this folder

@Geod24
Copy link
Member

Geod24 commented Jul 31, 2019

Not necessary due to common handling with other mixins. It's only necessary to test what is different.

This line is not shared, and what I wanted to have covered by tests.

@WalterBright
Copy link
Member Author

That line looks green to me, and parseArguments() is called in 13 other places in parse.d.

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.

7 participants