Fix for issue 11947 - typecons.Proxy incorrectly handles variadic member templates #1914
Conversation
|
In the future, please copy-paste the title of the bug you're fixing into the commit/pull request description. This way nobody has to look up/memorize the bug numbers. It's also helpful to put a link to the bug report in the pull request description. I've added it to this one. |
|
Thanks for the heads up, I'll be more thorough from now on. |
| auto ref opDispatch(this X, Args...)(auto ref Args args) | ||
| { | ||
| static if (Args.length) | ||
| // T is empty, deduction is in effect |
There was a problem hiding this comment.
Why this assumption? IFTI doesn't have to be used for all template parameters.
|
I am still acclimatizing to github. Shall I close this request and make a new one, or can I just commit new changes to further the review? |
Either amend the existing commit with new changes (then force push), or add new commits that should be squashed before pulling, once review is done. Alternatives to amending include pushing a remade branch entirely with the same name, and using interactive rebase with the |
|
Got it, thanks. I'm discovering even more quirks with this issue, i.e. forwarding when both template and non-template functions/opCall are present, or when making a proxy for a function. |
| auto ref opCall(this X, Args...)(auto ref Args args) { return a(args); } | ||
| auto ref opCall(this X, Args...)(auto ref Args args) | ||
| { | ||
| static if(is(typeof(a.opCall(args)))) |
There was a problem hiding this comment.
Forgot to mention: why this change? It seems like a no-op. If it's due to a bug, there should be a comment.
There was a problem hiding this comment.
I've commented on the opCall in the same report in bugzilla. Or you mean why the static if? Then a comment is due indeed. It's to allow proxying e.g. a function. The opCall will become even more elaborate to accomodate for all possibilities.
Do you recommend splitting this into separate issues? I.e. one for opDisptatch, one for opCall?
There was a problem hiding this comment.
I mean the existing code, i.e. a(args) should be correct. I notice that it doesn't work for pointers to structs with opCall, which seems to be the underlying problem as presented in the bug report. Automatic dereferencing of pointers is only a thing for the dot/member-access operator, which precludes opCall and all other operator overloads (which makes sense - stuff like unary *, indexing and binary + etc. would conflict with those of the pointer type).
If you want to proxy a struct by reference, make get return by ref.
There was a problem hiding this comment.
Oh, so you mean that attempting to opCall() a pointer is an entirely separate problem (bug?). Ok, makes sense.
But that still won't cancel out the need for various static ifs in the upcoming fix, to accomodate for: templated opCall() (exactly the same problem as with opDispatch, manifests when partially specifying template arguments; the "easily broken" problem you mentioned), proxying a function, mixed templated/non-templated opCall declarations. What a mess :)
|
Okay, another round :) |
|
Umm... You're commenting on an outdated commit. Is this intentional? Although the comment about mismatch in template/real argument numbers does seem interesting. I haven't accounted for that case. Oh well, more work, I guess. |
Seems that way... It wasn't intentional. Don't know how I managed that.
Yeah, I was looking at your commit "as a whole", and I think your |
|
Not MegreArgs per se. An additional check is needed to see if |
| // variadic template method | ||
| static assert(is(typeof(impostor.variadic()))); | ||
| static assert(is(typeof(impostor.variadic("hello")))); | ||
| static assert(is(typeof(impostor.variadic!int(42, "hello")))); |
There was a problem hiding this comment.
This test is wrong! Replacing variadic with p.variadic causes it to fail.
I suggest that you prefix each test with a variant which bypasses Proxy and addresses RealThing directly, to avoid false positives.
There was a problem hiding this comment.
I'm not sure I understand what you mean.
There was a problem hiding this comment.
This line will not work:
impostor.p.variadic!int(42, "hello");
Note that by using .p, we address RealThing directly, bypassing Proxy.
But your test asserts that this line will work:
impostor.variadic!int(42, "hello");
Proxy's goal is to forward valid uses of an object to the alias it is proxying. However, the line doesn't work if you remove the Proxy and address the RealThing directly, so it is not a valid way to use RealThing.
Thus, there is no obligation for Proxy to work in this case. Adding the line to the test implies that you assert that any future implementation of Proxy needs to satisfy this false requirement. I also suspect that this false requirement is partially responsible for the size and complexity of your patch.
There was a problem hiding this comment.
Ah, I see now. Yes, that is indeed a false assumption.
However, the size and complexity of the patch stems from the fact that it tries to deal with several possible scenarios at once:
- template members
- variadic template members
- templates overloading non-template members
Perhaps it's indeed feasible to break this up in a series of patches which would help to reason about each one, as you've suggested in your pull request. I don't mind closing this one in favour of yours and later submitting incremental fixes. What do you think?
There was a problem hiding this comment.
However, the size and complexity of the patch stems from the fact that it tries to deal with several possible scenarios at once:
All right. MergeArgs was what got me suspicious. But I haven't looked closely yet.
I don't mind closing this one in favour of yours and later submitting incremental fixes. What do you think?
You could do it here, just rebase the pull and split the changes into separate commits.
|
ping |
|
No updates as of yet. I was away from D for a while, only now resuming the work on these changes. |
|
Note that I have a second pull request which attempts to solve the same problem: #1980 |
|
ping |
|
Since the other PR #1980 has merged, and this one hasn't been updated for a while, I'm closing this for now. Please reopen this once you have time to work on an improvement to the fix already checked in. |
https://d.puremagic.com/issues/show_bug.cgi?id=11947