Issue 18314 - std.traits.getSymbolsByUDA only considers the first sym… #6085
Conversation
|
Thanks for your pull request, @Biotronic! 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
|
Biotronic
force-pushed
the
Issue-18314
branch
from
9173971
to
e1486c8
Compare
std/traits.d
Outdated
| @@ -7856,6 +7833,49 @@ template getSymbolsByUDA(alias symbol, alias attribute) | |||
| alias getSymbolsByUDA = membersWithUDA; | |||
| } | |||
|
|
|||
| template getSymbolsByUDAImpl(alias symbol, alias attribute, names...) | |||
|
(The Jenkins + CircleCI failures are unrelated) |
Biotronic
force-pushed
the
Issue-18314
branch
from
e1486c8
to
53346a1
Compare
|
The thing I dislike about my implementation is |
|
Thanks for the PR @Biotronic. I was planning on fixing it myself, but you were way too fast 😄 (I reported it like 24 hours ago) |
std/traits.d
Outdated
| void foo(int a); | ||
| } | ||
|
|
||
| assert(getSymbolsByUDA!(A, lal).length == 2); |
There was a problem hiding this comment.
Right. This should be done at CT.
std/traits.d
Outdated
|
|
||
| struct A | ||
| { | ||
| @lal |
There was a problem hiding this comment.
insert this declaration first
@lal
void foo()(string){}update the assertions accordingly, and have fun.
There was a problem hiding this comment.
For the sake of style, could we use @Attr1 and @Attr2 instead of @lol and @lal? I know I originally used those in my issue report, but it definitely was not my intention that such funny names ended up in the source code.
There was a problem hiding this comment.
This bug has already been filed in BugZilla. Looks to be by you:
https://issues.dlang.org/show_bug.cgi?id=16206
It's not the mandate of this PR to fix bugs in DMD. That said, mentioning the bug in the test is probably a good idea.
There was a problem hiding this comment.
Indeed. I planned to put the link to the issue after a possible "wtf" reaction from your part. But things didn't happen as expected.
There was a problem hiding this comment.
WTF?!
Here. Feeling better now?
There was a problem hiding this comment.
I originally used those in my issue report, but it definitely was not my intention that such funny names ended up in the source code.
If you already come with the style, it should be attr1 and attr2 - https://dlang.org/dstyle.html#naming_udas
But meh, I don't have a preference between these two. It's quite typical for contributors to put the code from the issue 1:1 into Phobos. Also it doesn't end up in the user documentation.
And if you disagree, this can always be improved. It shouldn't block this PR.
There was a problem hiding this comment.
Indeed. I planned to put the link to the issue after a possible "wtf" reaction from your part. But things didn't happen as expected.
Please refrain from wasting valuable time of other contributors in the future.
A simple comment:
I just bumped into a similar issue. See XYZ
would have saved all parties time...
There was a problem hiding this comment.
Sure, but we'd have less fun that way.
…bol of an overload set
Biotronic
force-pushed
the
Issue-18314
branch
from
53346a1
to
20c3e7f
Compare
|
And apparently I broke the auto-merge when I addressed the stuff that was mentioned above. |
Yes, it's not a bug it's a security feature.
|
…bol of an overload set
This change expands overload sets such that each overload is considered separately.