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.algorithm cleanup #2996

Merged
merged 7 commits into from
Feb 17, 2015
Merged

std.algorithm cleanup #2996

merged 7 commits into from
Feb 17, 2015

Conversation

quickfur
Copy link
Member

  • Move forward to std.functional
  • Move SortOutput to std.algorithm.sorting
  • Cleanup rest of detritus in std/algorithm/package.d.

@andralex
Copy link
Member

Thanks! Was forward available from std.algorithm before this change? If so, you may want to insert a public import in order to not break code.

@andralex
Copy link
Member

Was forward available from std.algorithm before this change?

Meant to say: was it available in 2.066?

@quickfur
Copy link
Member Author

Hmm, git history indicates that it was added in 2012, so we definitely need to forward it to the new location. I'm uneasy about using a public import, though... wouldn't that pollute std.algorithm with std.functional symbols? It might also cause new conflicts. Is there a way to publicly import a specific symbol?

H. S. Teoh added 2 commits February 16, 2015 13:04
Add forwarding alias to std.functional.forward to prevent breaking
existing code.
@quickfur
Copy link
Member Author

Also, should there be a deprecation message to point users to the new location of forward?

@schveiguy
Copy link
Member

wouldn't that pollute std.algorithm with std.functional symbols?

It appears that it already is that way, as your change removes that import. Can you statically import it, and than alias the symbol?

Is there a way to publicly import a specific symbol?

I think you can simply import the symbol you want directly, no?

should there be a deprecation message to point users to the new location of forward

Yes, so probably the right way to do this is to use a deprecated alias.

@quickfur
Copy link
Member Author

I used a private import. Is a static import better? What side effects might it have? I remember that imports have some quirky semantics when it comes to visibility.

And for some reason, adding a deprecation clause to the alias doesn't seem to trigger a deprecation error even when compiling with -de. Is this a known bug?

@quickfur
Copy link
Member Author

Alright, I'm adding it anyway, since I don't have time to babysit this all day. At least when the compiler is fixed the message will appear. For now it will just be a transparent alias.

@schveiguy
Copy link
Member

used a private import. Is a static import better? What side effects might it have?

I think a static import prevents the import of std.functional's symbols into std.algorithm's namespace. Basically, you have to say std.functional.x every time.

The deprecation message should work, I tested it on a small example, exactly as you have it, and a message is printed.

@schveiguy
Copy link
Member

Note that if you happen to have imported std.functional elsewhere (not via std.algorithm), you will not hit the alias.

@quickfur
Copy link
Member Author

Maybe my compiler is broken, I'll try to recompile git HEAD and see if the message appears. In the meantime, I've changed it to a static import with a deprecation message. Should be ready to merge if there are no other issues. The deprecation message issue, if it's actually a real issue, can be done separately, no need to hold up this PR just because of that.

@kuettler
Copy link
Contributor

I guess, it is the import std.functional; in e.g. comparison.d, which is publicly imported in package.d, that might be in the way of the deprecation message.

@kuettler
Copy link
Contributor

Seems good to me.

@andralex
Copy link
Member

Auto-merge toggled on

andralex added a commit that referenced this pull request Feb 17, 2015
@andralex andralex merged commit 5dd6df0 into dlang:master Feb 17, 2015
@quickfur quickfur deleted the std_algo_cleanup branch February 17, 2015 03:31
@schveiguy
Copy link
Member

that might be in the way of the deprecation message.

yes, that's likely it. In reality, this means the alias will not be hit, since importing std.algorithm means you will import std.algorithm.comparison. However, in the event that private imports actually do become private, then the alias will be effective.

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.

4 participants