Fix Issue 17908 - Can't alias an overload set with disabled function#7244
Fix Issue 17908 - Can't alias an overload set with disabled function#7244dlang-bot merged 1 commit intodlang:masterfrom
Conversation
|
Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla references
|
test/compilable/test17908b.d
Outdated
| void main() | ||
| { | ||
| g(10); | ||
| } |
There was a problem hiding this comment.
I don't see any use in this test case (also it's wrong ATM).
There was a problem hiding this comment.
I wanted to make sure that changing the order of the declarations will not influence the result (in the original bug report, one workaround in the code would have been to change the order of the function declarations.)
There was a problem hiding this comment.
@RazvanN7 it's simple to just put it in the same module as the other test - use a different name than foo.
There was a problem hiding this comment.
Yeah putting them all in one file when possible is the recommended approach (more files == slower autotester).
If you test for the order of declaration, then it makes sense. Could you add a comment before the second test case when you move them together, so it's more obvious to the clueless reader ?
Also, any specific reasons some don't have a body ?
There was a problem hiding this comment.
Could you add a comment before the second test case when you move them together, so it's more obvious to the clueless reader ?
Sure thing.
Also, any specific reasons some don't have a body ?
No, it was just a slip on my part.
test/fail_compilation/test17908.d
Outdated
| void main() | ||
| { | ||
| g(10); | ||
| } |
There was a problem hiding this comment.
Could you add a test with one member of the overload set being disabled (out of two), and calling the disabled overload ?
There was a problem hiding this comment.
Done, thanks for the tip
RazvanN7
force-pushed
the
Issue_17908
branch
2 times, most recently
from
8dc2bee to
3f4b3b2
Compare
src/ddmd/dsymbol.d
Outdated
| return; | ||
| } | ||
| } | ||
| error(loc, "is not callable because it is annotated with @disable"); |
There was a problem hiding this comment.
Nitpick: Indent is off
test/fail_compilation/test17908b.d
Outdated
|
|
||
| void foo(); | ||
| @disable void foo(int) {} | ||
| alias g = foo; |
There was a problem hiding this comment.
Could you also merge this file with the previous one, and add a body for foo ?
|
@mathias-lang-sociomantic Thanks for the review. Hope that all is good now. |
|
It looks like you messed up the test. There's two fail_compilation where there should only be one, and the compilable test disappeared. |
|
@mathias-lang-sociomantic You said to merge fail_compilation/test17908b.d with the previous one which could mean either the compilable one or fail_compilation/test17908a.b. I assumed it's the compilable one since if I merge the 2 fail compilation ones you get only the first error encountered, not both. |
|
In test17908b only the third check should fail, the first two should pass - no test for the latter. |
Hum sorry I thought you'd get both failure. But it's probably better to keep them separated then (as before). Sorry for that! |
|
@mathias-lang-sociomantic No probs, it was just a misunderstanding. |
|
Hum Jenkins seems broken, I see that failure on other P.R. as well :( |
@mathias-lang-sociomantic yes because we had a regression in phobos. We fixed that problem in dlang/phobos#5816, so we should be good now. |
|
And unsurprisingly, the CI was red but no attention was given to it. I hope it's made mandatory sooner than later. |
6 participants
Checking a disabled aliased symbol was done by calling the checkDeprecated function from the Dsymbol class. If the symbol was disabled, then an error was issued. I added the checking for functions in the same overload set. If an overload which is not disabled is found, then we're good, otherwise, issue error.