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.

Sign up for GitHub

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

Jump to bottom

Tracking issue for PathBuf capacity methods #58234

Closed
Mark-Simulacrum opened this issue Feb 6, 2019 · 17 comments · Fixed by #71328
Closed

Tracking issue for PathBuf capacity methods #58234

Mark-Simulacrum opened this issue Feb 6, 2019 · 17 comments · Fixed by #71328
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. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Feb 6, 2019 •
edited
Loading

@Mark-Simulacrum Mark-Simulacrum added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. C-feature-accepted Category: A feature request that has been accepted pending implementation. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Feb 6, 2019
@Mark-Simulacrum Mark-Simulacrum mentioned this issue Feb 6, 2019
Open
@aaronstillwell
Copy link
Contributor

aaronstillwell commented Feb 6, 2019 •
edited
Loading

Happy to take this on as a first contribution to rust.

@Mark-Simulacrum

This comment has been minimized.

Sign in to view
@aaronstillwell
Copy link
Contributor

@Mark-Simulacrum are you able to define a list of the methods that need to be implemented as part of this PR?

@Mark-Simulacrum
Copy link
Member Author

I think all of these will essentially call the underlying functions on OsString.

  • capacity
  • with_capacity
  • clear
  • reserve
  • reserve_exact
  • shrink_to_fit
  • shrink_to -- this one is more of a maybe, since it's unstable on OsString. We can reuse the same tracking issue for this.

We'll probably want to add all of these under the same feature gate.

@aaronstillwell
Copy link
Contributor

@Mark-Simulacrum thanks for that. I have the implementation drafted locally. I'm having some difficulty however in running the tests for stdlib - even a cargo check is producing errors not related to my changes. Is there any documentation I've missed for best practices on compiling / working on stdlib?

@Mark-Simulacrum

This comment has been minimized.

Sign in to view
@ordovicia

This comment has been minimized.

Sign in to view
@aaronstillwell aaronstillwell mentioned this issue Feb 17, 2019
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 17, 2019
@GuillaumeGomez
Rollup merge of rust-lang#58539 - aaronstillwell:master, r=Mark-Simul…
12bccb4 
…acrum

Add alias methods to PathBuf for underlying OsString (rust-lang#58234)

Implemented the following methods on PathBuf which forward to the underlying OsString.

- capacity
- with_capacity
- clear
- reserve
- reserve_exact
- shrink_to_fit
- shrink_to

These methods have been documented with reference to the original docs for `OsString`. @Mark-Simulacrum please let me know if you feel this does not suffice.

Further, I've not yet included tests for these definitions - please advise on how comprehensive tests need to be for methods such as these that simply alias other (already tested) methods.

(This PR addresses issue rust-lang#58234)
@GuillaumeGomez GuillaumeGomez mentioned this issue Feb 17, 2019
Closed
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Feb 18, 2019
@Mark-Simulacrum
Rollup merge of rust-lang#58539 - aaronstillwell:master, r=Mark-Simul…
e882771 
…acrum

Add alias methods to PathBuf for underlying OsString (rust-lang#58234)

Implemented the following methods on PathBuf which forward to the underlying OsString.

- capacity
- with_capacity
- clear
- reserve
- reserve_exact
- shrink_to_fit
- shrink_to

These methods have been documented with reference to the original docs for `OsString`. @Mark-Simulacrum please let me know if you feel this does not suffice.

Further, I've not yet included tests for these definitions - please advise on how comprehensive tests need to be for methods such as these that simply alias other (already tested) methods.

(This PR addresses issue rust-lang#58234)
@Mark-Simulacrum Mark-Simulacrum mentioned this issue Feb 18, 2019
Closed
@kennytm kennytm mentioned this issue Feb 19, 2019
Merged
kennytm added a commit to kennytm/rust that referenced this issue Feb 20, 2019
@kennytm
Rollup merge of rust-lang#58539 - aaronstillwell:master, r=Mark-Simul…
c965858 
…acrum

Add alias methods to PathBuf for underlying OsString (rust-lang#58234)

Implemented the following methods on PathBuf which forward to the underlying OsString.

- capacity
- with_capacity
- clear
- reserve
- reserve_exact
- shrink_to_fit
- shrink_to

These methods have been documented with reference to the original docs for `OsString`. @Mark-Simulacrum please let me know if you feel this does not suffice.

Further, I've not yet included tests for these definitions - please advise on how comprehensive tests need to be for methods such as these that simply alias other (already tested) methods.

(This PR addresses issue rust-lang#58234)
bors added a commit that referenced this issue Feb 20, 2019
@bors
Auto merge of #58578 - kennytm:rollup, r=kennytm
ff5dc27 
Rollup of 24 pull requests

Successful merges:

 - #56470 (Modify doctest's auto-`fn main()` to allow `Result`s)
 - #58044 (Make overflowing and wrapping negation const)
 - #58303 (Improve stability tags display)
 - #58336 (Fix search results interactions)
 - #58384 (Fix tables display)
 - #58392 (Use less explicit shifting in std::net::ip)
 - #58409 (rustdoc: respect alternate flag when formatting impl trait)
 - #58456 (Remove no longer accurate diagnostic code about NLL)
 - #58528 (Don't use an allocation for ItemId in StmtKind)
 - #58530 (Monomorphize less code in fs::{read|write})
 - #58534 (Mention capping forbid lints)
 - #58536 (Remove UB in pointer tests)
 - #58538 (Add missing fmt structs examples)
 - #58539 (Add alias methods to PathBuf for underlying OsString (#58234))
 - #58544 (Fix doc for rustc "-g" flag)
 - #58545 (Add regression test for a specialization-related ICE (#39448))
 - #58546 (librustc_codegen_llvm => 2018)
 - #58551 (Explain a panic in test case net::tcp::tests::double_bind)
 - #58553 (Use more impl header lifetime elision)
 - #58562 (Fix style nits)
 - #58565 (Fix typo in std::future::Future docs)
 - #58568 (Fix a transposition in driver.rs.)
 - #58569 (Reduce Some Code Repetitions like `(n << amt) >> amt`)
 - #58576 (Stabilize iter::successors and iter::from_fn)
bors added a commit that referenced this issue Feb 20, 2019
@bors
Auto merge of #58578 - kennytm:rollup, r=kennytm
f66e469 
Rollup of 24 pull requests

Successful merges:

 - #56470 (Modify doctest's auto-`fn main()` to allow `Result`s)
 - #58044 (Make overflowing and wrapping negation const)
 - #58303 (Improve stability tags display)
 - #58336 (Fix search results interactions)
 - #58384 (Fix tables display)
 - #58392 (Use less explicit shifting in std::net::ip)
 - #58409 (rustdoc: respect alternate flag when formatting impl trait)
 - #58456 (Remove no longer accurate diagnostic code about NLL)
 - #58528 (Don't use an allocation for ItemId in StmtKind)
 - #58530 (Monomorphize less code in fs::{read|write})
 - #58534 (Mention capping forbid lints)
 - #58536 (Remove UB in pointer tests)
 - #58538 (Add missing fmt structs examples)
 - #58539 (Add alias methods to PathBuf for underlying OsString (#58234))
 - #58544 (Fix doc for rustc "-g" flag)
 - #58545 (Add regression test for a specialization-related ICE (#39448))
 - #58546 (librustc_codegen_llvm => 2018)
 - #58551 (Explain a panic in test case net::tcp::tests::double_bind)
 - #58553 (Use more impl header lifetime elision)
 - #58562 (Fix style nits)
 - #58565 (Fix typo in std::future::Future docs)
 - #58568 (Fix a transposition in driver.rs.)
 - #58569 (Reduce Some Code Repetitions like `(n << amt) >> amt`)
 - #58576 (Stabilize iter::successors and iter::from_fn)
@bheesham

This comment has been minimized.

Sign in to view
@Mark-Simulacrum Mark-Simulacrum added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. and removed C-feature-accepted Category: A feature request that has been accepted pending implementation. labels Mar 1, 2019
@Mark-Simulacrum Mark-Simulacrum changed the title Add PathBuf capacity methods Tracking issue for PathBuf capacity methods Mar 1, 2019
@Mark-Simulacrum Mark-Simulacrum removed the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Mar 1, 2019
@Mark-Simulacrum

This comment has been minimized.

Sign in to view
@Mark-Simulacrum
Copy link
Member Author

Reviewing this since then, I think it may be worth removing these APIs and replacing them with something like AsRef<OsString> for PathBuf and AsMut. These aren't really commonly used so hiding them as such seems reasonable and is less heavy.

@jonas-schievink jonas-schievink added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Nov 26, 2019
@kornelski
Copy link
Contributor

I'd love to see these stabilized. I have a tool that manipulates lots of paths, so I'd like to avoid needless reallocations.

  • I generate lots of temporary paths in a loop. PathBuf.clear() + PathBuf.push() would work nicely to reuse the buffer.

  • I need to concatenate several components. I know their length, so I could use PathBuf::with_capacity()

  • I want to store lots of paths for later. Box<Path> is a bit smaller than PathBuf, but if I can't reserve exact capacity, into() will reallocate.

@Amanieu
Copy link
Member

Amanieu commented Apr 8, 2020

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 8, 2020 •
edited by SimonSapin
Loading

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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 proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 8, 2020
@dtolnay
Copy link
Member

dtolnay commented Apr 8, 2020

@rfcbot reviewed
assuming we're only stabilizing capacity + with_capacity + clear + reserve + reserve_exact + shrink_to_fit; and shrink_to is being moved to #56431 instead.

@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 Apr 9, 2020
@rfcbot
Copy link

rfcbot commented Apr 9, 2020

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

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Apr 19, 2020
@rfcbot
Copy link

rfcbot commented Apr 19, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 19, 2020
@Mark-Simulacrum Mark-Simulacrum mentioned this issue Apr 19, 2020
Merged
@Mark-Simulacrum
Copy link
Member Author

I've posted a stabilization PR in #71328.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 20, 2020
@Dylan-DPC
Rollup merge of rust-lang#71328 - Mark-Simulacrum:stabilize-pathbuf-c…
55510ff 
…apacity, r=sfackler

Stabilize PathBuf capacity methods

Closes rust-lang#58234.

Stabilization FCP finished in rust-lang#58234 (comment).
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 20, 2020
@Dylan-DPC
Rollup merge of rust-lang#71328 - Mark-Simulacrum:stabilize-pathbuf-c…
793c669 
…apacity, r=sfackler

Stabilize PathBuf capacity methods

Closes rust-lang#58234.

Stabilization FCP finished in rust-lang#58234 (comment).
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 20, 2020
@Dylan-DPC
Rollup merge of rust-lang#71328 - Mark-Simulacrum:stabilize-pathbuf-c…
156ed10 
…apacity, r=sfackler

Stabilize PathBuf capacity methods

Closes rust-lang#58234.

Stabilization FCP finished in rust-lang#58234 (comment).
@bors bors closed this as completed in 7561714 Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Stabilize PathBuf capacity methods Mark-Simulacrum/rust
9 participants
@kornelski @bheesham @Amanieu @jonas-schievink @dtolnay @Mark-Simulacrum @aaronstillwell @ordovicia @rfcbot