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

Prelude: rename and consolidate extension traits #18559

Merged
merged 1 commit into from
Nov 6, 2014

Conversation

aturon
Copy link
Member

@aturon aturon commented Nov 3, 2014

This commit renames a number of extension traits for slices and string slices, now that they have been refactored for DST. In many cases, multiple extension traits could now be consolidated. Further consolidation will be possible with generalized where clauses.

The renamings are consistent with the new -Prelude suffix. There are probably a few more candidates for being renamed this way, but that is left for API stabilization of the relevant modules.

Because this renames traits, it is a:

[breaking-change]

However, I do not expect any code that currently uses the standard library to actually break.

Closes #17917

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

#[doc(no_inline)] pub use str::{Str, StrVector, StrSlice};
#[doc(no_inline)] pub use str::{IntoMaybeOwned, StrAllocating, UnicodeStrSlice};
#[doc(no_inline)] pub use str::{Str, StrVector, StrPrelude};
#[doc(no_inline)] pub use str::{IntoMaybeOwned, StrAllocating, UnicodeStrPrelude};
Copy link
Member

Choose a reason for hiding this comment

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

Would this be a good time to consolidate Str and StrPrelude? (or can we not do that?)

Copy link
Member Author

Choose a reason for hiding this comment

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

People are definitely using Str for generics in libraries (for better or worse), so I don't want to lose it until we have a more general replacement (which I have some thoughts about, but for a different PR.)

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, good point!

@alexcrichton
Copy link
Member

Love seeing MutableSlice go away!

pub trait MutableSliceAllocating<T> for Sized? {
/// Allocating extension methods for slices on Ord values.
#[experimental = "likely to merge with other traits"]
pub trait OrdSliceAllocPrelude<T> for Sized? {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, could this be called OrdSlicePrelude, and then we could use modules/renaming to import both this and the libcore version into the prelude?

Copy link
Member

Choose a reason for hiding this comment

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

That may not help the docs, however.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, so this is why I was thinking it might be nice for std to just define its own set of extension traits that hook into the underlying traits from core etc. I can't see a way to do that without duplicating the docs, though :-/

@aturon
Copy link
Member Author

aturon commented Nov 3, 2014

Pushed a fix for some of your changes, but looks like at least one of my comments got lost. I definitely agree that the names here are... terrible... but that's a little intentional (e.g with the Prelude) suffix since you shouldn't ever need to type these, and we're trying to make more clear the role they're playing.

That said, generalized where clauses will help a lot in further collapsing these traits and @nikomatsakis says they're not too far away.

Let me know whether you prefer the strategy I've taken here, or something with more concise names and where std only exports its own, bigger extension traits. (The only downside I see in the latter is the doc duplication.)

@aturon
Copy link
Member Author

aturon commented Nov 3, 2014

@alexcrichton Oh, the other comment that got eaten: the story with AsSlice is similar to Str: people are using this for generic programming, and we need a more general replacement (rather than folding into these prelude traits).

@alexcrichton
Copy link
Member

Sounds good to me, r=me with a rebase.

@alexcrichton
Copy link
Member

We can discuss pursuing more aggressive options later, but this is certainly a good step forward

@aturon aturon force-pushed the prelude_cleanup branch 2 times, most recently from e6ff48a to 8db009e Compare November 5, 2014 19:31
This commit renames a number of extension traits for slices and string
slices, now that they have been refactored for DST. In many cases,
multiple extension traits could now be consolidated. Further
consolidation will be possible with generalized where clauses.

The renamings are consistent with the [new `-Prelude`
suffix](rust-lang/rfcs#344). There are probably
a few more candidates for being renamed this way, but that is left for
API stabilization of the relevant modules.

Because this renames traits, it is a:

[breaking-change]

However, I do not expect any code that currently uses the standard
library to actually break.

Closes rust-lang#17917
bors added a commit that referenced this pull request Nov 6, 2014
This commit renames a number of extension traits for slices and string slices, now that they have been refactored for DST. In many cases, multiple extension traits could now be consolidated. Further consolidation will be possible with generalized where clauses.

The renamings are consistent with the [new `-Prelude` suffix](rust-lang/rfcs#344). There are probably a few more candidates for being renamed this way, but that is left for API stabilization of the relevant modules.

Because this renames traits, it is a:

[breaking-change]

However, I do not expect any code that currently uses the standard library to actually break.

Closes #17917
@alexcrichton alexcrichton merged commit cfafc1b into rust-lang:master Nov 6, 2014
@alexcrichton
Copy link
Member

I pushed this manually because all tests passed and @bors is having a tough time catching up.

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.

prev_permutation / next_permutation and the Permutations iterator should be in libcore
4 participants