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

Implement slice_strip feature #73414

Merged
merged 1 commit into from
Jul 2, 2020
Merged

Implement slice_strip feature #73414

merged 1 commit into from
Jul 2, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Jun 16, 2020

Tracking issue: #73413

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 16, 2020
@Mark-Simulacrum
Copy link
Member

Does making the impls here heterogeneous (i.e. impl<T> [T] { fn strip_prefix<U>(&self, prefix: &[U]); } hurt inference sufficiently that we don't want to do that?

@jonas-schievink
Copy link
Contributor

Unsure why I'm assigned, I'm not on the libs team. r? @Mark-Simulacrum

@tesuji
Copy link
Contributor Author

tesuji commented Jun 16, 2020

hurt inference sufficiently

I don't understand the question.
Why would we want this?

impl<T> [T] { 
    fn strip_prefix<U>(&self, prefix: &[U]); 
}

@tesuji
Copy link
Contributor Author

tesuji commented Jun 16, 2020

Also, the strip_* methods somewhat copy the signature of slice::starts_with.

Does starts_with also cause problems?

pub fn starts_with(&self, needle: &[T]) -> bool

@tesuji tesuji changed the title Implement slice_strip feature Implement slice_strip feature Jun 16, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Jun 17, 2020

r? @SimonSapin

@tesuji
Copy link
Contributor Author

tesuji commented Jun 25, 2020

r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned SimonSapin Jun 25, 2020
@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 25, 2020
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks! I am on board with this. It matches the method names on str and it matches the starts/ends_with signatures on slice.

@dtolnay
Copy link
Member

dtolnay commented Jun 25, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jun 25, 2020

📌 Commit 560a996 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 25, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 25, 2020
Implement `slice_strip` feature

Tracking issue: rust-lang#73413
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 26, 2020
Implement `slice_strip` feature

Tracking issue: rust-lang#73413
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 26, 2020
Implement `slice_strip` feature

Tracking issue: rust-lang#73413
@Manishearth
Copy link
Member

test time.rs - time::Duration::zero (line 145) ... ok
test unit.rs - unit::() (line 8) ... ok

failures:

---- slice/mod.rs - slice::[T]::strip_prefix (line 1468) stdout ----
error[E0308]: mismatched types
---- slice/mod.rs - slice::[T]::strip_suffix (line 1519) stdout ----
error[E0308]: mismatched types
 --> slice/mod.rs:1522:1
  |
6 | assert_eq!(v.strip_suffix(&[]), Some(v));
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected slice `[{integer}]`, found array `[{integer}; 3]`
  |
  = note: expected enum `std::option::Option<&[{integer}]>`
             found enum `std::option::Option<&[{integer}; 3]>`
  = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
Couldn't compile the test.

failures:
    slice/mod.rs - slice::[T]::strip_prefix (line 1468)
    slice/mod.rs - slice::[T]::strip_prefix (line 1479)
    slice/mod.rs - slice::[T]::strip_suffix (line 1508)
    slice/mod.rs - slice::[T]::strip_suffix (line 1519)

test result: FAILED. 2600 passed; 4 failed; 28 ignored; 0 measured; 0 filtered out

#73733 (comment)

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 26, 2020
Comment on lines 1478 to 1485
///
/// ```
/// #![feature(slice_strip)]
/// let v = &[10, 40, 30];
/// assert_eq!(v.strip_prefix(&[]), Some(v));
/// let v: &[u8] = &[];
/// assert_eq!(v.strip_prefix(&[]), Some(v));
/// ```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this example needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it.

@tesuji tesuji force-pushed the slice_strip branch 2 times, most recently from 83f5819 to 6068504 Compare June 26, 2020 03:51
Comment on lines 1464 to 1465
/// This method returns the original slice if `prefix` is an empty slice
/// or returns [`None`] if slice does not start with `prefix`.
Copy link
Member

Choose a reason for hiding this comment

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

I think the wording of this was clearer in a previous revision. Slice not starting with prefix is a scenario that is important to specify; this says what the meaning of Option in the return type is. Empty prefix is a small clarification (not even an edge case, since the behavior falls out from the rest of the documentation). I think the emphasis in this paragraph is backward and not conducive to recognizing the part that is more important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I repharsed it. Could you take a look?

@tesuji tesuji requested a review from dtolnay July 1, 2020 12:39
@dtolnay
Copy link
Member

dtolnay commented Jul 2, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jul 2, 2020

📌 Commit cd9d833 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 2, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 2, 2020
…arth

Rollup of 10 pull requests

Successful merges:

 - rust-lang#73414 (Implement `slice_strip` feature)
 - rust-lang#73564 (linker: Create GNU_EH_FRAME header by default when producing ELFs)
 - rust-lang#73622 (Deny unsafe ops in unsafe fns in libcore)
 - rust-lang#73684 (add spans to injected coverage counters, extract with CoverageData query)
 - rust-lang#73812 (ast_pretty: Pass some token streams and trees by reference)
 - rust-lang#73853 (Add newline to rustc MultiSpan docs)
 - rust-lang#73883 (Compile rustdoc less often.)
 - rust-lang#73885 (Fix wasm32 being broken due to a NodeJS version bump)
 - rust-lang#73903 (Changes required for rustc/cargo to build for iOS targets)
 - rust-lang#73938 (Optimise fast path of checked_ops with `unlikely`)

Failed merges:

r? @ghost
@bors bors merged commit 63d392e into rust-lang:master Jul 2, 2020
@tesuji tesuji deleted the slice_strip branch July 2, 2020 16:38
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants