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.math: Begin deprecation of accidentally leaked AliasSeq #7487

Merged
merged 1 commit into from
Jul 3, 2020

Conversation

ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented May 18, 2020

No, there isn't a better way of doing this. Complements to @schveiguy for an initial version of this PR.

Rebased and updated following dmd fix.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @ibuclaw!

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#7487"

@ibuclaw ibuclaw force-pushed the stdmath/aliasseq branch from 4a016a0 to b3da400 Compare May 18, 2020 13:01
@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented May 18, 2020

That workaround is really ugly...

The weird part is that this bug doesn't apply to other deprecated imports, e.g function (templates).

@dnadlinger
Copy link
Member

dnadlinger commented May 18, 2020

Maybe std.meta.AliasSeq was unintentionally available through std.math and will be removed after …. Please import std.meta instead. or something that makes clear where the symbol came from? If I saw this as an user not too familiar with metaprogramming in D, I would try to look up docs for std.math.AliasSeq to figure out what was going on.

@schveiguy
Copy link
Member

The worst part about this is that the deprecation message will be printed for the mixin line, not the usage. However, one possible saving grace is that the full instantiation is printed, so you might be able to deduce where it's coming from based on that.

std/math.d Outdated Show resolved Hide resolved
@ibuclaw ibuclaw force-pushed the stdmath/aliasseq branch from b3da400 to c3360bc Compare May 18, 2020 15:21
@schveiguy
Copy link
Member

schveiguy commented May 18, 2020

I don't see a deprecation in the D-YAML build log. Was the import of std.meta added? It would be nice to see that this actually works ;)

Edit: yes it was... dlang-community/D-YAML#254

schveiguy
schveiguy previously approved these changes May 18, 2020
@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented May 18, 2020

Please don't merge this yet, I think i have a fix for the missing deprecation.

EDIT: See dlang/dmd#11158

std/math.d Outdated
public import std.meta : AliasSeq;
private template DeprecatedAliasSeq(V...)
{
deprecated("std.meta.AliasSeq was unintentionally available from std.math and will be removed after 2.102. Please import std.meta instead")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to break this line in half, style check complains.

@schveiguy schveiguy dismissed their stale review May 19, 2020 14:39

I'll reapprove if the DMD PR doesn't work out. Just don't want this to be merged prematurely.

@schveiguy
Copy link
Member

Should we close this now that dlang/dmd#11158 is merged?

@MoonlightSentinel
Copy link
Contributor

Not really, just add a plain deprecation message?

@schveiguy
Copy link
Member

Right, maybe adjust this to properly deprecate the import.

@12345swordy
Copy link

@ibuclaw ping

@ibuclaw ibuclaw force-pushed the stdmath/aliasseq branch from c3360bc to ab414ef Compare July 2, 2020 23:31
@ibuclaw
Copy link
Member Author

ibuclaw commented Jul 2, 2020

Rebased.

@dlang-bot dlang-bot merged commit 22ebc70 into dlang:master Jul 3, 2020
@ibuclaw ibuclaw deleted the stdmath/aliasseq branch July 3, 2020 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants