Skip to content
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

Merged
merged 1 commit into from
Aug 1, 2020

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Jul 29, 2020

In support of the DMD PR dlang/dmd#11474 , which will be red until this is pulled.

Commit message follows:

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`.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @Geod24!

Bugzilla references

Your 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 locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7570"

@Geod24 Geod24 force-pushed the add-stc-in branch 2 times, most recently from 6ef80e4 to d66c05e Compare July 29, 2020 09:04
@Geod24
Copy link
Member Author

Geod24 commented Jul 31, 2020

Can we get this merged ? Without the DMD PR it's harmless, so we can always revert it.

Copy link
Member

@wilzbach wilzbach left a 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`.
@wilzbach wilzbach merged commit a4d8029 into dlang:master Aug 1, 2020
@Geod24 Geod24 deleted the add-stc-in branch August 1, 2020 14:27
out_ = 0x04, /// ditto
lazy_ = 0x08, /// ditto
scope_ = 0x10, /// ditto
return_ = 0x20, /// ditto
Copy link
Contributor

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.

@JohanEngelen
Copy link
Contributor

@Geod24 @wilzbach @thewilsonator
Can we please revert the change of the values in the ParameterStorageClass enum? I.e. make the values correspond to:

    none    = 0,
    scope_  = 1,    /// ditto
    out_    = 2,    /// ditto
    ref_    = 4,    /// ditto
    lazy_   = 8,    /// ditto
    return_ = 0x10, /// ditto
    in_     = 0x20, /// ditto

@Geod24
Copy link
Member Author

Geod24 commented Apr 6, 2021

@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 ?
The point about the re-ordering is taken, the change should have been proposed independently and not tied to the addition of in.

@JohanEngelen
Copy link
Contributor

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).
I think we'll solve it at Weka, turning the magic numbers (phobos) into stable Weka numbers using a simple conversion function.

Geod24 pushed a commit to JohanEngelen/phobos that referenced this pull request Apr 6, 2021
JohanEngelen added a commit to JohanEngelen/phobos that referenced this pull request Apr 13, 2021
dlang-bot pushed a commit that referenced this pull request Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants