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

Add CStr::bytes iterator #104353

Merged
merged 2 commits into from
Mar 14, 2024
Merged

Add CStr::bytes iterator #104353

merged 2 commits into from
Mar 14, 2024

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Nov 13, 2022

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now core::ffi::c_str::Bytes instead of core::ffi::CStrBytes.

@rustbot
Copy link
Collaborator

rustbot commented Nov 13, 2022

r? @thomcc

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 13, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 13, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Nov 13, 2022
@rust-log-analyzer

This comment has been minimized.

@thomcc
Copy link
Member

thomcc commented Nov 13, 2022

This seems fairly reasonable to me. Can you file an ACP though?

@thomcc thomcc added the S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. label Nov 13, 2022
@clarfonthey
Copy link
Contributor Author

Sounds reasonable to me! Filed those and replaced the description with a link back to the ACP.

@Dylan-DPC Dylan-DPC removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2023
@clarfonthey
Copy link
Contributor Author

Since the ACP was accepted, I removed the as_c_str and AsRef impl and opened a tracking issue. Adding the removed bits was mentioned as an unanswered question in the tracking issue since it was not accepted as part of the ACP.

@clarfonthey
Copy link
Contributor Author

clarfonthey commented May 30, 2023

@rustbot ready

(not sure if I can just do this, or if someone else will have to)

(edit: looks like I can, but it doesn't remove the S-waiting-on-ACP tag. someone else will have to do that)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2023
@clarfonthey
Copy link
Contributor Author

@rustbot blocked

Going to open a PR in light of this ACP being accepted: rust-lang/libs-team#134

Then just make this depend on that one being merged first.

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2023
@Dylan-DPC Dylan-DPC removed the S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. label Jul 28, 2023
@bors
Copy link
Contributor

bors commented Sep 20, 2023

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

@thomcc
Copy link
Member

thomcc commented Feb 1, 2024

I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks.

r? libs

@rustbot rustbot assigned joshtriplett and unassigned thomcc Feb 1, 2024
@joshtriplett
Copy link
Member

r? libs

@rustbot rustbot assigned cuviper and unassigned joshtriplett Feb 11, 2024
@clarfonthey
Copy link
Contributor Author

Oh, I'm terrible with batching review comments, so, I get that. Will poke around the code in a bit to see what I can do.

Comment on lines +510 to +516
/// We could eventually expose this publicly, if we wanted.
#[inline]
#[must_use]
const fn as_non_null_ptr(&self) -> NonNull<c_char> {
NonNull::from(&self.inner).as_non_null_ptr()
}

Copy link
Contributor Author

@clarfonthey clarfonthey Mar 12, 2024

Choose a reason for hiding this comment

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

Decided to put this because it seems like something we'd probably add in the future, but I don't really feel like doing an ACP for this right now. It felt reasonable enough to factor into its own method, at least.

I thought about why as_ptr uses addr_of! instead of a safe version (since, well, we require a reference to &self to make this work, and thus any potential UB arguments don't make sense) and concluded that we don't need to here, but if I'm wrong about that, please let me know!

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what you're referring to -- CStr::as_ptr() just calls the slice as_ptr(), which is just a couple pointer casts. I only found addr_of! used in Rc/Arc::as_ptr, and those are digging into a further heap allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, I totally mixed up the implementations for as_ptr and to_bytes_with_nul. The latter uses addr_of to convert the slice into *const [u8] instead of the existing method, but that's going to be unsafe anyway, so, it's fair.

Copy link
Member

Choose a reason for hiding this comment

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

Huh, I don't see any reason it's needed there either, just a little bit shorter than it was with double pointer casts before addr_of!. No matter.

@cuviper
Copy link
Member

cuviper commented Mar 13, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 13, 2024

📌 Commit a38a556 has been approved by cuviper

It is now in the queue for this repository.

@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 Mar 13, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 13, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#104353 (Add CStr::bytes iterator)
 - rust-lang#120699 (Document `TRACK_DIAGNOSTIC` calls.)
 - rust-lang#121207 (Add `-Z external-clangrt`)
 - rust-lang#122397 (Various cleanups around the const eval query providers)
 - rust-lang#122416 (Various style improvements to `rustc_lint::levels`)
 - rust-lang#122422 (compiletest: Allow `only-unix` in test headers)
 - rust-lang#122424 (fix: typos)
 - rust-lang#122425 (Increase timeout for new bors bot)
 - rust-lang#122426 (Fix StableMIR `WrappingRange::is_full` computation)
 - rust-lang#122430 (Generate link to `Local` in `hir::Let` documentation)
 - rust-lang#122434 (pattern analysis: rename a few types)
 - rust-lang#122437 (pattern analysis: remove `MaybeInfiniteInt::JustAfterMax`)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 13, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 13, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104353 (Add CStr::bytes iterator)
 - rust-lang#120699 (Document `TRACK_DIAGNOSTIC` calls.)
 - rust-lang#120943 (Create some minimal HIR for associated opaque types)
 - rust-lang#121764 (Make incremental sessions identity no longer depend on the crate names provided by source code)
 - rust-lang#122375 (CFI: Break tests into smaller files)
 - rust-lang#122397 (Various cleanups around the const eval query providers)
 - rust-lang#122405 (Add methods to create StableMIR constant)
 - rust-lang#122416 (Various style improvements to `rustc_lint::levels`)
 - rust-lang#122440 (const-eval: organize and extend tests for required-consts)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 14, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#104353 (Add CStr::bytes iterator)
 - rust-lang#114038 (unix time module now return result)
 - rust-lang#119676 (rustdoc-search: search types by higher-order functions)
 - rust-lang#120699 (Document `TRACK_DIAGNOSTIC` calls.)
 - rust-lang#121899 (Document how removing a type's field can be bad and what to do instead)
 - rust-lang#121940 (Mention Register Size in `#[warn(asm_sub_register)]`)
 - rust-lang#122397 (Various cleanups around the const eval query providers)
 - rust-lang#122405 (Add methods to create StableMIR constant)
 - rust-lang#122416 (Various style improvements to `rustc_lint::levels`)
 - rust-lang#122440 (const-eval: organize and extend tests for required-consts)
 - rust-lang#122461 (fix unsoundness in Step::forward_unchecked for signed integers)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 14, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 14, 2024
…iper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104353 (Add CStr::bytes iterator)
 - rust-lang#119676 (rustdoc-search: search types by higher-order functions)
 - rust-lang#120699 (Document `TRACK_DIAGNOSTIC` calls.)
 - rust-lang#121899 (Document how removing a type's field can be bad and what to do instead)
 - rust-lang#122405 (Add methods to create StableMIR constant)
 - rust-lang#122416 (Various style improvements to `rustc_lint::levels`)
 - rust-lang#122421 (Improve `Step` docs)
 - rust-lang#122440 (const-eval: organize and extend tests for required-consts)
 - rust-lang#122461 (fix unsoundness in Step::forward_unchecked for signed integers)

Failed merges:

 - rust-lang#122397 (Various cleanups around the const eval query providers)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1ae69ae into rust-lang:master Mar 14, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2024
Rollup merge of rust-lang#104353 - clarfonthey:cstr-bytes-iter, r=cuviper

Add CStr::bytes iterator

See rust-lang/libs-team#135 for an ACP.

Since rust-lang/libs-team#134 was also accepted, this type is now `core::ffi::c_str::Bytes` instead of `core::ffi::CStrBytes`.
@clarfonthey clarfonthey deleted the cstr-bytes-iter branch March 21, 2024 02:17
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 Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants