Fix IFTI with named arguments #15040
Conversation
|
Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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 referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "stable + dmd#15040" |
dkorpel
changed the title
dkorpel
force-pushed
the
named-arg-ifti
branch
from
017b459
to
b0eeafb
Compare
dkorpel
force-pushed
the
named-arg-ifti
branch
from
b0eeafb
to
43220d8
Compare
dkorpel
force-pushed
the
named-arg-ifti
branch
2 times, most recently
from
3118235
to
c4dff12
Compare
dkorpel
changed the title
dkorpel
force-pushed
the
named-arg-ifti
branch
from
c4dff12
to
d226699
Compare
dlang-bot
added
Review:stalled
Review:Needs Rebase
and removed
Review:Needs Rebase
Review:stalled
labels
dkorpel
force-pushed
the
named-arg-ifti
branch
from
d226699
to
27cbe83
Compare
dkorpel
force-pushed
the
named-arg-ifti
branch
from
27cbe83
to
56c11f6
Compare
|
I think this is ready to go. There's future improvements for error messages and tuples, but I don't want this PR to grow bigger, and all the recent refactoring commits keep pulling out the rug from under this PR so I'd like to get this in before I have to rebase again :) |
dkorpel
removed
the
Review:WIP
| @@ -5723,8 +5754,7 @@ extern (C++) final class TemplateMixin : TemplateInstance | |||
|
|
|||
| /************************************ | |||
| * This struct is needed for TemplateInstance to be the key in an associative array. | |||
| * Fixing https://issues.dlang.org/show_bug.cgi?id=15812 and | |||
| * https://issues.dlang.org/show_bug.cgi?id=15813 would make it unnecessary. | |||
| * Fixing https://issues.dlang.org/show_bug.cgi?id=15813 would make it unnecessary. | |||
There was a problem hiding this comment.
This change, although correct, seems unrelated. I just want to make sure this change is intended and not the left overs of a different branch.
There was a problem hiding this comment.
It's intended, I'm updating outdated or confusing comments as I come across them. (Though I won't mix unrelated code changes of course)
| /* | ||
| TEST_OUTPUT: | ||
| --- | ||
| fail_compilation/named_arguments_ifti_error.d(17): Error: template `f` is not callable using argument types `!()(int, int)` |
There was a problem hiding this comment.
This error message does not seem to accurately explain what the problem is. I would expect something along the lines of: "Missing argument for parameter 'y'".
There was a problem hiding this comment.
The problem is that the current template instantiation code gags / discards most error messages, requiring a refactor to make more informative errors. I don't want to add that to this PR, but want to look into improving this next (which is long overdue; template errors are a big source of confusion especially with ref and lambda parameters).
|
|
||
| void main() | ||
| { | ||
| f(x: 3, x: 3); |
There was a problem hiding this comment.
Is this a typo? Did you mean 'y' for the second 'x' ? Either way, the error for assigning a value to 'x' twice is misleading.
There was a problem hiding this comment.
This is a fail compilation test, it's testing that wrong use of names doesn't succeed in instantiating. I added comments to the test case to clarify.
|
Is this worth retargeting for stable/the current 2.108 beta? |
|
cc @dkorpel |
|
Yes, this was meant for 2.108, so targeting stable now. |
|
Is this supposed to contain 5 commits, 4 of which look unrelated? |
|
No, I don't know why they are there. Git says it's up to date with stable. |
deduceFunctionTemplateMatchmakes the assumption that the arguments and parameters arrays are parallel, andresolveNamedArgumentsdoesn't handle template tuple parameters yet. Also, default parameters can come beforenfargsnow, so the logic comparingargiwithnfargshas to be rewritten.