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

Tracking issue for {Vec,String}::splice #32310

Closed
alexcrichton opened this issue Mar 17, 2016 · 15 comments
Closed

Tracking issue for {Vec,String}::splice #32310

alexcrichton opened this issue Mar 17, 2016 · 15 comments
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Tracking issue for rust-lang/rfcs#1432.

First needs to be implemented, then needs to go through FCP!

@alexcrichton alexcrichton added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 17, 2016
@aturon aturon added the E-help-wanted Call for participation: Help is requested to fix this issue. label Feb 6, 2017
@aturon
Copy link
Member

aturon commented Feb 6, 2017

There's a preliminary implementation here, which could easily be picked up and brought over the finish line!

@brson brson added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Mar 1, 2017
@alexcrichton alexcrichton added B-unstable Blocker: Implemented in the nightly compiler and unstable. and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. labels Apr 24, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 24, 2017
Implement Vec::splice and String::splice (RFC 1432)

RFC: rust-lang/rfcs#1432, tracking issue: rust-lang#32310
A rebase of rust-lang#32355 with a few more tests.

Let me know if you have any ideas for more tests.

cc @SimonSapin
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 24, 2017
Implement Vec::splice and String::splice (RFC 1432)

RFC: rust-lang/rfcs#1432, tracking issue: rust-lang#32310
A rebase of rust-lang#32355 with a few more tests.

Let me know if you have any ideas for more tests.

cc @SimonSapin
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 24, 2017
Implement Vec::splice and String::splice (RFC 1432)

RFC: rust-lang/rfcs#1432, tracking issue: rust-lang#32310
A rebase of rust-lang#32355 with a few more tests.

Let me know if you have any ideas for more tests.

cc @SimonSapin
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 25, 2017
Implement Vec::splice and String::splice (RFC 1432)

RFC: rust-lang/rfcs#1432, tracking issue: rust-lang#32310
A rebase of rust-lang#32355 with a few more tests.

Let me know if you have any ideas for more tests.

cc @SimonSapin
bors added a commit that referenced this issue Apr 25, 2017
Implement Vec::splice and String::splice (RFC 1432)

RFC: rust-lang/rfcs#1432, tracking issue: #32310
A rebase of #32355 with a few more tests.

Let me know if you have any ideas for more tests.

cc @SimonSapin
@clarfonthey
Copy link
Contributor

Because this is implemented, is there anything blocking stabilisation?

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jul 22, 2017
@aturon
Copy link
Member

aturon commented Jul 25, 2017

@clarcharr Nope! I'll get the wheels in motion:

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jul 25, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 25, 2017
@dtolnay
Copy link
Member

dtolnay commented Jul 30, 2017

Why would you ever use the return value from String::splice?

  1. The String and the replacement str are in scope (start of 'a and 'b).
  2. Call splice, receive Splice struct.
  3. The String is exclusively borrowed and unusable.
  4. Process the replaced segment one char at a time.
  5. Drop the Splice struct.
  6. Drop the String and the replacement str (end of 'a and 'b, must happen after 5).

Given that 'a and 'b are required to outlive Splice, wouldn't it always be simpler and at least as efficient to do this?

  1. The String and the replacement str are in scope (start of 'a and 'b).
  2. Grab the soon replaced segment &s[range].
  3. The String is shared borrowed but otherwise usable.
  4. Process the soon replaced segment one char at a time or as an ordinary &str.
  5. Call splice which eagerly does the replace and returns ().
  6. Drop the String and the replacement str (end of 'a and 'b).

@rfcbot concern return value of String::splice

@dtolnay
Copy link
Member

dtolnay commented Jul 30, 2017

If IndexAssign (or IndexSet or whatever, rust-lang/rfcs#997) existed today, would we be using that instead? Would we want both?

let mut s: String = /* ... */;
s[from..to] = "replacement";

@rfcbot concern IndexAssign

@clarfonthey
Copy link
Contributor

I will say that splice would still be useful for Vec, just not for String in that case, because IndexAssign still won't be as efficient.

Perhaps going forward the features for Vec::splice and String::splice could be split, allowing to stabilise the former will keeping the latter unstable?

@sfackler
Copy link
Member

sfackler commented Aug 9, 2017

I'm on board with stabilizing the Vec method, but not String for the reasons mentioned above. I think IndexSet is far enough away that I'd be comfortable landing this and then deprecating it in if/when that stabilizes.

@dtolnay
Copy link
Member

dtolnay commented Aug 22, 2017

I agree that IndexSet is too far away to block this.

@rfcbot resolved IndexAssign

@dtolnay
Copy link
Member

dtolnay commented Aug 22, 2017

I filed #44038 to follow up on changing the signature of String::splice.

@rfcbot resolved return value of String::splice

@dtolnay
Copy link
Member

dtolnay commented Aug 22, 2017

The following applies only to Vec::splice.

@rfcbot reviewed

@rfcbot
Copy link

rfcbot commented Aug 23, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Aug 23, 2017
@kennytm
Copy link
Member

kennytm commented Aug 23, 2017

IMO if String::splice is not returning the previous data, the method shouldn't be really called splice.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Aug 30, 2017
…lnay

Remove Splice struct return value from String::splice

The implementation is now almost identical to the one in the RFC.

Fixes rust-lang#44038
cc rust-lang#32310
@rfcbot
Copy link

rfcbot commented Sep 2, 2017

The final comment period is now complete.

@dtolnay
Copy link
Member

dtolnay commented Sep 17, 2017

The FCP above applies to Vec::splice. I filed #44643 as the tracking issue for String::splice which remains unstable with its new signature.

budziq added a commit to budziq/rust that referenced this issue Sep 17, 2017
alexcrichton added a commit to alexcrichton/rust that referenced this issue Sep 18, 2017
Stabilized vec_splice and modified splice tracking issue

This stabilizes the vec_splice (Vec part of splice RFC)
Fixes rust-lang#32310.
alexcrichton added a commit to alexcrichton/rust that referenced this issue Sep 18, 2017
Stabilized vec_splice and modified splice tracking issue

This stabilizes the vec_splice (Vec part of splice RFC)
Fixes rust-lang#32310.
dtolnay pushed a commit to dtolnay/rust that referenced this issue Sep 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants