-
Notifications
You must be signed in to change notification settings - Fork 37
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 #141: Improve type inference for macros, in particular handle type arguments. #178
Fix #141: Improve type inference for macros, in particular handle type arguments. #178
Conversation
f5aa6a4
to
9654914
Compare
auto result = ExpressionContext.make(context); | ||
result.params = params; | ||
return result; | ||
} |
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.
Is there a reason why constructors are not used?
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.
Yes, you cannot create (allocate on the heap) the associative array in the constructor.
@jacob-carlborg What about making a little review : ) ? |
Yes, sorry. I've been busy with non-D related things. |
|
||
extern (D) auto FOO(T)(auto ref T x) | ||
{ | ||
return BAR(int); |
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.
This is translated like this because at this point we don't know what BAR
is?
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.
Yes, do you think BAR!int() is better?
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.
Well, BAR(int);
doesn't compile, regardless if BAR
is defined in the D code or not. So yes I think BAR!int()
is better.
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.
It turns out that it isn't that trivial to change it (more than 1h of work), can we merge it as is for now? I don't have time to fix it right now. Maybe I'll manage after winter...
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.
Sure, can you rebase please.
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.
Do you remember what you meant by rebase? There is no merge conflict after all.
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.
No. The only thing I can think of is perhaps it's behind master.
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.
Rebased.
9654914
to
37b4dcf
Compare
…lar handle type arguments.
37b4dcf
to
8dae476
Compare
Thanks. |
The commit learns dstep to infer the signature of the macro basing on usage. It is still quite rudimentary implementation but it handles the cases that were wrong before anyway. Example:
More examples in unit tests.