Skip to content

Allow comparisons between CStr, CString, and Cow<CStr>. #137268

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow comparisons between CStr, CString, and Cow<CStr>. #137268

wants to merge 1 commit into from
+123 −0

Conversation

bjoernager
Copy link
Contributor

@bjoernager bjoernager commented Feb 19, 2025 •
edited
Loading

Closes: #137265

This PR adds the trait implementations proposed in the ACP under the c_string_eq_c_str feature gate:

// core::ffi

impl PartialEq<&Self> for CStr;

impl PartialEq<CString> for CStr;

impl PartialEq<Cow<'_, Self>> for CStr;

// alloc::ffi

impl PartialEq<CStr> for CString;

impl PartialEq<&CStr> for CString;

impl PartialEq<Cow<'_, CStr>> for CString;

// alloc::borrow

impl PartialEq<CStr> for Cow<'_, CStr>;

impl PartialEq<&CStr> for Cow<'_, CStr>;

impl PartialEq<CString> for Cow<'_, CStr>;

As I understand it, stable traits cannot be unstably implemented for stable types, and we would thereby be forced to skip the FCP and directly stabilise these implementations (as is done in this PR).

(@joshtriplett mentioned that Crater may have to be run).
@rustbot
Copy link
Collaborator

rustbot commented Feb 19, 2025

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 19, 2025
@bjoernager bjoernager mentioned this pull request Feb 19, 2025
Open
2 tasks
@bjoernager
Copy link
Contributor Author

@rustbot label +T-libs-api -T-libs

r? libs-api
@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 19, 2025
@rustbot rustbot assigned BurntSushi and unassigned Amanieu Feb 19, 2025
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2025
@rustbot

This comment has been minimized.

Sign in to view
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2025
@bjoernager bjoernager changed the title Implement PartialEq<{&Self, CString}> for CStr and PartialEq<{CStr, &CStr}> for CString. Feb 19, 2025
@bjoernager
Implement 'PartialEq<{&Self, CString, Cow<Self>}>' for 'CStr'; Implem…
817b093 
…ent 'PartialEq<{CStr, &CStr, Cow<CStr>}>' for 'CString'; Implement 'PartialEq<{CStr, &CStr, CString}>' for 'Cow<CStr>';
@bjoernager
Copy link
Contributor Author

Friendly ping @BurntSushi :)
@jieyouxu
Copy link
Member

r? libs-api
@rustbot rustbot assigned Amanieu and unassigned BurntSushi Apr 28, 2025
@Amanieu
Copy link
Member

Amanieu commented Apr 30, 2025

@bors try
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2025
@bors
Auto merge of rust-lang#137268 - bjoernager:c-string-eq-c-str, r=<try>
1f308f1 
Allow comparisons between `CStr`, `CString`, and `Cow<CStr>`.

Closes: rust-lang#137265

This PR adds the trait implementations proposed in the [ACP](rust-lang/libs-team#517) under the `c_string_eq_c_str` feature gate:

```rust
// core::ffi

impl PartialEq<&Self> for CStr;

impl PartialEq<CString> for CStr;

impl PartialEq<Cow<'_, Self>> for CStr;

// alloc::ffi

impl PartialEq<CStr> for CString;

impl PartialEq<&CStr> for CString;

impl PartialEq<Cow<'_, CStr>> for CString;

// alloc::borrow

impl PartialEq<CStr> for Cow<'_, CStr>;

impl PartialEq<&CStr> for Cow<'_, CStr>;

impl PartialEq<CString> for Cow<'_, CStr>;
```

As I understand it, stable traits cannot be unstably implemented for stable types, and we would thereby be forced to skip the FCP and directly stabilise these implementations (as is done in this PR).

(`@joshtriplett` mentioned that Crater may have to be run).
@bors
Copy link
Collaborator

bors commented Apr 30, 2025

⌛ Trying commit 817b093 with merge 1f308f1...
@bors
Copy link
Collaborator

bors commented Apr 30, 2025

☀️ Try build successful - checks-actions
Build commit: 1f308f1 (1f308f12738d8cc40bb23bf0c8114b37c164bb49)
@Amanieu
Copy link
Member

Amanieu commented Apr 30, 2025

@craterbot check
@craterbot
Copy link
Collaborator

👌 Experiment pr-137268 created and queued.
🤖 Automatically detected try build 1f308f1
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-137268 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
@craterbot
Copy link
Collaborator

🎉 Experiment pr-137268 is completed!
📊 3 regressed and 3 fixed (623643 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 2, 2025
@Amanieu
Copy link
Member

Amanieu commented May 3, 2025

@rfcbot merge
@rfcbot
Copy link
Collaborator

rfcbot commented May 3, 2025 •
edited by m-ou-se
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 May 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
8 participants
@bjoernager @rustbot @jieyouxu @Amanieu @bors @craterbot @rfcbot @BurntSushi