-
-
Notifications
You must be signed in to change notification settings - Fork 655
Fix1215 #4516
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
Fix1215 #4516
Conversation
Please follow the existing dmd code style:
You can use |
Thanks for the review. Style fixed, please take another look. |
Is this supported? |
It's not indeed, thanks for the catch ! Here's a fixed version. |
} | ||
else if (t) | ||
{ | ||
index->error(loc, "Expected an expression as index, got a type (%s)", t->toChars()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DMD error messages (user-visible ones, anyway) generally start with a lower-case letter.
Sorry for pestering you with yet another round of stylistic nit-picks, but here you go. ;) I'll defer to Kenji for the actual technical review. |
Please do comment, this is my first contribution to dmd, so this is useful :) |
Sorry for late reply. I reviewed code, and confirmed that the code is enough to support a indexed type with dot identifier. It looks not smart, but I know that comes from the current dmd internal design. To make things more clean, we need to improve the Type structure design around TypeQualifier, and I think it's not so important at this first step. LGTM after the code style and test case tweaks. @legrosbuffle To support the indexed type with dot identifier, we also need to update D grammer. Can you also work for that? If you cannot open a grammer update PR in parallel, I'll file a website bugzilla issue after merging this. |
} | ||
|
||
case DYNCAST_EXPRESSION: | ||
sm = takeTypeTupleIndex(loc, sc, s, id, (Expression*)id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all casts, insert a space before *
.
s/(Expression*)id
/(Expression *)id
/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
See the corresponding pull request here: dlang/dmd#4516
@@ -0,0 +1,118 @@ | |||
// PERMUTE_ARGS: | |||
// REQUIRED_ARGS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally compilable test is compiled with -c
and generates objfile. But the test for 1215 does not have to do it.
So, you can list -o-
in REQUIRED_ARGS:
to stop backend codegen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks for the suggestion.
Thanks for the comments ! I've also created a PR for the grammar on dlang.org. |
LGTM. Thanks for your awesome work! |
Auto-merge toggled on |
This should have been squashed down to one or two commits before it was merged. |
…iled. If a TypeQualifier is not trivial dot-chain of scope and member symbols, it will be resolved as the equivalent expression. The new TypeQualified structure introduced in dlang#4516 for issue 1215 is now fully in `TypeQualified::resolveExprType`, and the ICE issue will be solved in it.
…iled. If a TypeQualifier is not trivial dot-chain of scope and member symbols, it will be resolved as the equivalent expression. The new TypeQualified structure introduced in dlang#4516 for issue 1215 is now fully in `TypeQualified::resolveExprType`, and the ICE issue will be solved in it.
Parse Args[0].T as a type in:
struct S(Args...) {
Args[0].T m;
alias T = Args[0].T;
}
See bug 1215 for context.