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

Stabilize CString::from_vec_with_nul[_unchecked] #89292

Merged
merged 2 commits into from
Oct 21, 2021
Merged

Stabilize CString::from_vec_with_nul[_unchecked] #89292

merged 2 commits into from
Oct 21, 2021

Conversation

CleanCut
Copy link
Contributor

@CleanCut CleanCut commented Sep 27, 2021 •
edited by JohnTitor
Loading

Closes the tracking issue #73179. I am keeping this in draft mode until the FCP has ended.

This is my first time stabilizing a feature, so I would appreciate any guidance on things I should do differently.

Closes #73179

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2021
@CleanCut
Copy link
Contributor Author

Hmm, apparently rust-highfive doesn't distinguish between draft-mode PRs and regular ones. 😬 In the future, if I want to keep things low-key I'll wait until later to create the PR.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nagisa
Copy link
Member

nagisa commented Sep 27, 2021 •
edited
Loading

You can r? @ghost in the description when creating the PR to have highfive to ignore auto-assignment.

@CleanCut
Copy link
Contributor Author

@nagisa TIL, thanks!

@CleanCut CleanCut mentioned this pull request Sep 27, 2021
Closed
Lurk pushed a commit to Lurk/nsstring_leak that referenced this pull request Oct 2, 2021
found a way to get rid of libc
90913dd 
It turns out that using vec as a buffer works, and it is even more performant. And when rust-lang/rust#89292 will be merged, things will be even simpler (no pop from the buffer).
@joshtriplett joshtriplett added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2021
@joshtriplett
Copy link
Member

This seems reasonable to me. Tagging as waiting-on-fcp; the FCP is in the tracking issue.

@CleanCut CleanCut marked this pull request as ready for review October 4, 2021 23:33
@bors
Copy link
Contributor

bors commented Oct 11, 2021

☔ The latest upstream changes (presumably #89755) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 11, 2021
@bors
Copy link
Contributor

bors commented Oct 12, 2021

☔ The latest upstream changes (presumably #89813) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnTitor
Copy link
Member

FCP has been complete, could you rebase onto master instead of merging?

@JohnTitor JohnTitor removed the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Oct 20, 2021
@CleanCut
Copy link
Contributor Author

FCP has been complete, could you rebase onto master instead of merging?

Absolutely. Is there a bors way of doing this, or should I do it manually and force-push?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@CleanCut
Copy link
Contributor Author

@JohnTitor 🤔 I do not know what to do about the error that changing the version caused. Perhaps rebasing will fix it?

error[E0711]: feature `cstring_from_vec_with_nul` is declared stable since 1.58.0, but was previously declared stable since 1.57.0

@JohnTitor
Copy link
Member

Absolutely. Is there a bors way of doing this, or should I do it manually and force-push?

It exists but has some bugs/undesired behavior e.g. rust-lang/homu#158. So I recommend force-pushing.

@JohnTitor 🤔 I do not know what to do about the error that changing the version caused. Perhaps rebasing will fix it?

That's because you changed the version number as an additional commit, squashing commits into one should fix it.

@CleanCut CleanCut force-pushed the stabilize-cstring_from_vec_with_nul branch from 3a585f6 to 86b3dd9 Compare October 20, 2021 20:19
@CleanCut
Copy link
Contributor Author

✅ Squashed and rebased

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@CleanCut
Copy link
Contributor Author

🤔 I've squashed down to a single commit, but CI is still unhappy about the multiple version numbers.

@CleanCut @JohnTitor
fix 'since' version number
39af41e 
Co-authored-by: Yuki Okushi <jtitor@2k36.org>
@JohnTitor
Copy link
Member

Thanks! r+'ing in favor of FCP.
r? @JohnTitor @bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2021

📌 Commit 39af41e has been approved by JohnTitor

@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 Oct 20, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 21, 2021
@JohnTitor
Rollup merge of rust-lang#89292 - CleanCut:stabilize-cstring_from_vec…
504d16a 
…_with_nul, r=JohnTitor

Stabilize CString::from_vec_with_nul[_unchecked]

Closes the tracking issue rust-lang#73179. I am keeping this in _draft_ mode until the FCP has ended.

This is my first time stabilizing a feature, so I would appreciate any guidance on things I should do differently.

Closes rust-lang#73179
@JohnTitor JohnTitor mentioned this pull request Oct 21, 2021
Closed
@JohnTitor JohnTitor mentioned this pull request Oct 21, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2021
@bors
Auto merge of rust-lang#90119 - JohnTitor:rollup-e5t6khz, r=JohnTitor
e015ef5 
Rollup of 14 pull requests

Successful merges:

 - rust-lang#86984 (Reject octal zeros in IPv4 addresses)
 - rust-lang#87440 (Remove unnecessary condition in Barrier::wait())
 - rust-lang#88644 (`AbstractConst` private fields)
 - rust-lang#89292 (Stabilize CString::from_vec_with_nul[_unchecked])
 - rust-lang#90010 (Avoid overflow in `VecDeque::with_capacity_in()`.)
 - rust-lang#90029 (Add test for debug logging during incremental compilation)
 - rust-lang#90031 (config: add the option to enable LLVM tests)
 - rust-lang#90048 (Add test for line-number setting)
 - rust-lang#90071 (Remove hir::map::blocks and use FnKind instead)
 - rust-lang#90074 (2229 migrations small cleanup)
 - rust-lang#90077 (Make `From` impls of NonZero integer const.)
 - rust-lang#90097 (Add test for duplicated sidebar entries for reexported macro)
 - rust-lang#90098 (Add test to ensure that the missing_doc_code_examples is not triggered on foreign trait implementations)
 - rust-lang#90099 (Fix MIRI UB in `Vec::swap_remove`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 20687bb into rust-lang:master Oct 21, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 21, 2021
@CleanCut CleanCut deleted the stabilize-cstring_from_vec_with_nul branch October 21, 2021 19:13
@VictorKoenders VictorKoenders mentioned this pull request Oct 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnTitor JohnTitor JohnTitor approved these changes

Assignees

@JohnTitor JohnTitor

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
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.

Tracking Issue for methods to go from nul-terminated Vec<u8> to CString
8 participants
@CleanCut @rust-highfive @rust-log-analyzer @nagisa @joshtriplett @bors @JohnTitor @rustbot