Skip to content

Implement Borrow<CStr> for CString and ToOwned for CStr #26928

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

Merged
merged 2 commits into from
Jul 10, 2015

Conversation

reem
Copy link
Contributor

@reem reem commented Jul 9, 2015

This allows CString and CStr to be used with the Cow type,
which is extremely useful when interfacing with C libraries
that make extensive use of C-style strings.

@rust-highfive
Copy link
Contributor

r? @pcwalton

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

type Owned = CString;

fn to_owned(&self) -> CString {
unsafe { CString::from_vec_unchecked(self.to_bytes_with_nul().to_vec()) }
Copy link
Member

Choose a reason for hiding this comment

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

I think you want to_bytes here instead of to_bytes_with_nul because the from_vec_unchecked function pushes a nul byte on the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, updated.

@bluss
Copy link
Member

bluss commented Jul 9, 2015

Double-check due to Borrow: Eq and Hash implementations must match between CStr and CString. It may be that they don't hash alike since one is based on c_char data and the other on u8?

@reem
Copy link
Contributor Author

reem commented Jul 10, 2015

@bluss adding a test to check this.

This allows CString and CStr to be used with the Cow type,
which is extremely useful when interfacing with C libraries
that make extensive use of C-style strings.
@reem
Copy link
Contributor Author

reem commented Jul 10, 2015

Confirmed that they have equivalent hashes with a new test.

@bluss
Copy link
Member

bluss commented Jul 10, 2015

The test only checks bytes in the trivial range (0-127) where i8 and u8 overlap. I looked into i8's hash and I guess they should hash the same, but for the test to be real it needs to test byte values outside the common range.

@reem
Copy link
Contributor Author

reem commented Jul 10, 2015

Updated the test to include some higher values, still passes.

@bluss
Copy link
Member

bluss commented Jul 10, 2015

great, it's a good test to have in the codebase

@reem
Copy link
Contributor Author

reem commented Jul 10, 2015

r? @alexcrichton

@Gankra
Copy link
Contributor

Gankra commented Jul 10, 2015

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 10, 2015

📌 Commit 69579e4 has been approved by Gankro

bors added a commit that referenced this pull request Jul 10, 2015
This allows CString and CStr to be used with the Cow type,
which is extremely useful when interfacing with C libraries
that make extensive use of C-style strings.
@bors
Copy link
Collaborator

bors commented Jul 10, 2015

⌛ Testing commit 69579e4 with merge fddfd08...

@bors bors merged commit 69579e4 into rust-lang:master Jul 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants