-
-
Notifications
You must be signed in to change notification settings - Fork 609
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 Issue 17425 - add __traits(getParameterStorageClasses, f, i) #6829
Conversation
|
|
Blocking dlang/phobos#5427 |
|
@WalterBright there is some kind of error on osx32 |
Apparently another heisenbug in the testing scripts. |
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.
Looks fine to me.
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.
Just a few nits, otherwise LGTM.
| @@ -0,0 +1,18 @@ | |||
| add __traits(getParameterStorageClasses, f, i) | |||
|
|
|||
| $(LINK2 https://issues.dlang.org/show_bug.cgi?id=17425, Bugzilla 17425) | |||
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.
The changelog entry seems a bit terse. I'd use at least more descriptive variable names. Also, linking to the documentation instead of bugzilla might be more helpful.
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.
FYI the auto-tester allows to preview this since some time (though I am not sure why this isn't part of the diff, CC @CyberShadow)
In any case the link is e.g. http://dtest.dlang.io/artifact/website-72ba79b73f635fb08bdfe69d6826833c1c4c2ee0-a9052bf749bf29e4ae70eb23d51613de/web/changelog/2.075.0_pre.html#changelog17425 (the _pre file in the changelog folder).
FWIW there also seems to be a parenthesis problem in another changelog file..
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.
I didn't link to the documentation because that PR has not been pulled.
| @@ -968,6 +969,109 @@ extern (C++) Expression semanticTraits(TraitsExp e, Scope* sc) | |||
| auto se = new StringExp(e.loc, cast(char*)style); | |||
| return se.semantic(sc); | |||
| } | |||
| if (e.ident == Id.getParameterStorageClasses) | |||
| { | |||
| /* Accept a function symbol or a type, followed by a parameter index. | |||
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.
How about putting this code into a function? This improves readable and avoids making this function even larger (dmd is rather slow optimizing large functions).
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.
I leave refactorings to subsequent PRs, because they distract from what is being changed here.
| return new ErrorExp(); | ||
| } | ||
| ex = ex.ctfeInterpret(); | ||
| auto ii = ex.toUInteger(); |
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 doesn't propagate errors if the expression does not evaluate to an integer. That is not checked in other uses of toInteger/toUInteger elsewhere, too, though. Maybe add test pragma(msg, __traits(getParameterStorageClasses, foo, "1"));?
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.
That's a generic problem with all those functions, and fixing them should be in a separate PR.
Oops, diff was eaten by an overly voracious disk cleanup script. Fixed for future cleanups. |
The point of this is so that
std.traits.ParameterStorageClassTuple()can be implemented without needing to study the name mangling.