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

Add D_Preview<name> to identify enabled preview switches #11635

Closed
wants to merge 1 commit into from

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Aug 28, 2020

This will allow to adapt library code, e.g. Phobos, without having to observe the effect.
The downside is that those versions probably need to go through deprecation after the
flag is disabled, something which wouldn't be needed if the user was only observing the effect.

This will allow to adapt library code, e.g. Phobos, without having to observe the effect.
The downside is that those versions probably need to go through deprecation after the
flag is disabled, something which wouldn't be needed if the user was only observing the effect.
@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 + dmd#11635"

@wilzbach
Copy link
Member

wilzbach commented Aug 28, 2020

What about allowing only code from druntime to access these versions (or traits)?
Then it could be:

enum hasPreviewIn = __traits(hasPreviewFeature, "in");

And eventually it would become:

enum hasPreviewIn = true;

OTOH the same could be achieved without this intermediate step with a mentioned trait and it wouldn't necessarily require a deprecation phase as the trait could return true forever.

Edit: more importantly this also needs to take the -revert flags into account as well.

@Geod24
Copy link
Member Author

Geod24 commented Aug 28, 2020

What about allowing only code from druntime to access these versions (or traits)?

I also wanted to use it in other libraries (e.g. I would like to have Vibe.d ready when v2.094.0 is released).

OTOH the same could be achieved without this intermediate step with a mentioned trait and it wouldn't necessarily require a deprecation phase

True, I just thought a version would be less problematic. We don't want to balkanize the language with traits that stay there forever.

Edit: more importantly this also needs to take the -revert flags into account as well.

It checks that the field is set, and -revert will disable the field. I didn't add a version for DIP25 though.

@wilzbach
Copy link
Member

I also wanted to use it in other libraries (e.g. I would like to have Vibe.d ready when v2.094.0 is released).

import core.traits : hasPreviewIn should work, no?

True, I just thought a version would be less problematic. We don't want to balkanize the language with traits that stay there forever.

Well, the same argument can be made about introducing so many new version identifiers.
So I would prefer to just have one additional trait than many permanent versions that can never be removed.
In fact, there's already a bit of a precedent - LDC has a similar trait to query for what versions traditionally were used: __traits(targetHasFeature, ...):

https://github.com/ldc-developers/ldc/blob/1fa0be270ce730a4299d2913a725d197c6c822ce/tests/semantic/target_traits.d

@Geod24
Copy link
Member Author

Geod24 commented Aug 28, 2020

import core.traits : hasPreviewIn should work, no?

Not backward compatible.

Well, the same argument can be made about introducing so many new version identifiers.

True, but IMO their meaning is much clearer: D_Preview_in is easy to spot and grep for. Having a feature-based approach sure allows you to modify the code once, but it ends up being something that will live forever.

__traits(targetHasFeature, ...)

That is slightly different IMO. It makes sense to query the target capability (and also, those can be combined), while those version flags are intended only for a transitional period.

I could also remove the changelog entry if you prefer to keep it hidden. Or just bite it and close this, and detect if the feature is enabled with code.

@wilzbach
Copy link
Member

D_Preview_in is easy to spot and grep for. Having a feature-based approach sure allows you to modify the code once, but it ends up being something that will live forever.

I'm just worried that we will end up with a whole lot of versions that we can never remove once introduced.
What do other people think about this?

CC @atilaneves @PetarKirov @WalterBright

@Geod24
Copy link
Member Author

Geod24 commented Aug 31, 2020

I think I'm just going to table this, the workaround I have is good enough.

@Geod24 Geod24 closed this Aug 31, 2020
@Geod24 Geod24 deleted the preview-version branch August 31, 2020 13:58
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.

3 participants