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

WIP Partially stabilize StrExt for pattern-using methods #20058

Merged
merged 2 commits into from
Dec 29, 2014

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Dec 20, 2014

This stabilizes most methods on &str working with patterns in a way that is forwards-compatible with a generic string pattern matching API:

  • Methods that are using the primary name for their operation are marked as #[stable], as they can be upgraded to a full Pattern API later without existing code breaking. Example: contains(&str)
  • Methods that are using a more specific name in order to not clash with the primary one are marked as #[unstable], as they will likely be removed once their functionality is merged into the primary one. Example: contains_char<C: CharEq>(C)
  • The method docs got changed to consistently refer to the pattern types as a pattern.
  • Methods whose names do not match in the context of the more generic API got renamed. Example: trim_chars -> trim_matches

Additionally, all methods returning iterators got changed to return unique new types with changed names in accordance with the new naming guidelines.

See also rust-lang/rfcs#528

Due to some deprecations and type changes, this is a

[breaking-change]

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@Kimundi
Copy link
Member Author

Kimundi commented Dec 20, 2014

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned brson Dec 20, 2014
@Kimundi Kimundi force-pushed the str_pattern_pre branch 2 times, most recently from 36a46aa to 159245c Compare December 20, 2014 03:45
@aturon
Copy link
Member

aturon commented Dec 20, 2014

cc @alexcrichton I don't think I will have time to re-review this, but the initial draft looked good to me.

@alexcrichton
Copy link
Member

I will take a look at this soon, thanks in advance though @Kimundi!

@alexcrichton alexcrichton assigned alexcrichton and unassigned aturon Dec 21, 2014
@@ -1408,10 +1536,19 @@ pub trait StrPrelude for Sized? {
/// assert_eq!(v, vec![""]);
/// # }
/// ```
fn splitn<'a, Sep: CharEq>(&'a self, count: uint, sep: Sep) -> CharSplitsN<'a, Sep>;
#[stable]
#[inline]
Copy link
Member

Choose a reason for hiding this comment

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

I've generally used the #[inline] tag primarily to ensure that something can be inlined across crates, but because this is generic I think it will be instantiated across crates regardless. Did you need to add the #[inline] tag here (and a few places below) for other reasons?

Copy link
Member

Choose a reason for hiding this comment

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

Er but if you're just copying all the implementations from below up here don't worry about it!

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, for now I just copied inline attributes.

@alexcrichton
Copy link
Member

This looks fantastic, awesome work @Kimundi!

@Kimundi Kimundi force-pushed the str_pattern_pre branch 2 times, most recently from 49ae6a3 to 54bd7c9 Compare December 25, 2014 15:48
@Kimundi
Copy link
Member Author

Kimundi commented Dec 25, 2014

So, I rebased my branch, but have a few remaining questions/undecided methods:

  • Should I put work into making CharEq more general for now? (Eg, the impl CharEq for &mut P where P: CharEq proposal)
  • I marked match_indices as unstable for now, because its semantic might change to return an iterator over (uint, &str) (match, and its offset). The question is, should we do this change?
  • I had last minutes thoughts about the stability of find / rfind. In principle .find(foo) can be replaced with .match_indices(foo).map(|(i, _)| i).next(), so its not strictly necessary. On the one hand, we might want to keep it as convenience and for familiarity with other languages string APIs, which often provide such a function as low level building block. On the other hand, in Rust it wouldn't really be the low level building block, but just another consumer of the matcher iterators, so it might make sense to not provide it at all.
  • I also marked split_terminator as unstable for now, as I need to re-research whether it is a common function to provide in a languages string library. On option would be to make it a constructor function on Split itself, so that you could do .split(pat).terminator() - it would take self by value and construct a wrapper around the iterator type. The only issue here is that you could call it even after you already started iterating with the Split iterator, which would make it only well defined if called on a freshly constructed Split. (I think this was the reasoning that lead me to leave-in the terminator variations of split to begin with)
  • I left subslice_offset as unstable for now, as it doesn't really belong to the pattern-using methods on strings. The question is whether we want to support this operation at all for slices - if yes, then we probably should provide it for slice types in general, and it should probably get changed to return a Option<uint> instead of panicing.

@Kimundi Kimundi closed this Dec 25, 2014
@Kimundi
Copy link
Member Author

Kimundi commented Dec 25, 2014

Whoops

@Kimundi Kimundi reopened this Dec 25, 2014
Made iterator-returning methods return newtypes
Adjusted some docs to be forwards compatible with a generic pattern API
@aturon
Copy link
Member

aturon commented Dec 27, 2014

@Kimundi @alexcrichton I'm back from the holiday -- would you like me to take a separate reviewing pass over this PR?

@Kimundi
Copy link
Member Author

Kimundi commented Dec 28, 2014

I certainly wouldn't mind ;)

Mostly I'm interested in your and @alexcrichton's opinion about the questions I raised in that comment above.

@alexcrichton
Copy link
Member

Should I put work into making CharEq more general for now

Nah I think what we have right now is fine, adding more impls should be backwards compatible.

I marked match_indices as unstable for now ... should we do this change?

I think it's fine being #[unstable] for now, and we can focus on the API later when stabilizing it.

I had last minutes thoughts about the stability of find / rfind.

Due to match_indices being unstable for now, I think we need to call these #[stable]. I also do think that they're nice conveniences to have and we'd probably want them regardless.

I also marked split_terminator as unstable for now

Ok!

I left subslice_offset as unstable for now.

Sounds good to me!

Thanks again @Kimundi, this is all super awesome!

@alexcrichton
Copy link
Member

Ah I see find and rfind are also currently #[unstable], I think that's ok as a conservative approach, and they're easy enough to mark #[stable] soon as well!

bors added a commit that referenced this pull request Dec 29, 2014
This stabilizes most methods on `&str` working with patterns in a way that is forwards-compatible with a generic string pattern matching API:
- Methods that are using the primary name for their operation are marked as `#[stable]`, as they can be upgraded to a full `Pattern` API later without existing code breaking. Example: `contains(&str)`
- Methods that are using a more specific name in order to not clash with the primary one are marked as `#[unstable]`, as they will likely be removed once their functionality is merged into the primary one. Example: `contains_char<C: CharEq>(C)`
- The method docs got changed to consistently refer to the pattern types as a pattern.
- Methods whose names do not match in the context of the more generic API got renamed. Example: `trim_chars -> trim_matches` 

Additionally, all methods returning iterators got changed to return unique new types with changed names in accordance with the new naming guidelines.

See also rust-lang/rfcs#528

Due to some deprecations and type changes, this is a 

[breaking-change]
@bors bors merged commit c1f3aca into rust-lang:master Dec 29, 2014
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