-
-
Notifications
You must be signed in to change notification settings - Fork 725
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
std.traits: Add support for 'in' storage class #7570
Conversation
Thanks for your pull request, @Geod24! 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 "master + phobos#7570" |
6ef80e4
to
d66c05e
Compare
Can we get this merged ? Without the DMD PR it's harmless, so we can always revert it. |
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.
Fine by me
For a long time, the 'in' storage class was only a second class citizen, merely an alias for 'const', which is a type constructor. While it was also documented for a long time as being 'scope', this was removed when 'scope' actually started to have an effect (when DIP1000 started to be implemented). Currently, a switch (-preview=in) allows to get back this 'scope'. However, the 'in' storage class does not really exists, it gets lowered to 'const [scope]' at an early stage by the compiler, which means that we expose what is essentially an implementation detail to the user. There is a PR in DMD (dlang/dmd#11474) that aims to give 'in' an actual identity, instead of it being lowered to 'const [scope]'. The underlying motivation is to allow for extending 'in''s functionality, giving it the ability to pass by 'ref' when necessary, and accept rvalues. However, regardless of the second goal, having proper support for 'in' would lead to less confusing messages, better code generation, and less confusion w.r.t. the behavior of `std.traits.ParameterStorageClass`.
out_ = 0x04, /// ditto | ||
lazy_ = 0x08, /// ditto | ||
scope_ = 0x10, /// ditto | ||
return_ = 0x20, /// ditto |
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.
@Geod24 @wilzbach @thewilsonator
Please next time consider that changing the values of enum members can have a large impact on end users, for no clear benefit. The value changes were not needed, just adding in_ = 0x20
would have sufficed, with less user pain. As a practical example, the reassigning of these values means that at Weka we will have to recalculate ABI hashes for many functions.
…ameters See related: dlang#7570
@Geod24 @wilzbach @thewilsonator none = 0,
scope_ = 1, /// ditto
out_ = 2, /// ditto
ref_ = 4, /// ditto
lazy_ = 8, /// ditto
return_ = 0x10, /// ditto
in_ = 0x20, /// ditto |
@JohanEngelen : We could change the order to that if that works better for you, I personally don't have a horse in this race, but wouldn't that potentially cause the same problems for other users now that this has been released for two releases ? |
I guess one can also argue that users shouldn't rely on the values of the enum, and only use the value identifiers... Afaik dlang gives no guarantees about ABI compatibility between versions (i.e. shared stdlibs will not work as one might hope). |
…ameters See related: dlang#7570
…ameters See related: dlang#7570
…ameters See related: #7570
In support of the DMD PR dlang/dmd#11474 , which will be red until this is pulled.
Commit message follows: