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

AsRef and related conversions for CString #30616

Merged
merged 3 commits into from
Dec 31, 2015
Merged

Conversation

arcnmx
Copy link
Contributor

@arcnmx arcnmx commented Dec 29, 2015

Are trait impls still insta-stable? Considering that this design has been around for a long time on String and OsString it probably doesn't matter much...

The From impl is a bit strange to me. It's stolen from OsString but I'm not really sure about it... String just impls From<&str> instead, would that make more sense?

@rust-highfive
Copy link
Collaborator

r? @brson

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

@brson
Copy link
Contributor

brson commented Dec 29, 2015

cc @rust-lang/libs

@nagisa
Copy link
Member

nagisa commented Dec 29, 2015

Are trait impls still insta-stable?

Yes.

@@ -522,6 +522,37 @@ impl ToOwned for CStr {
}
}

#[unstable(feature = "cstring_asref", reason = "recently added", issue = "0")]
impl ops::Index<ops::RangeFull> for CString {
Copy link
Member

Choose a reason for hiding this comment

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

Probably should implement indexing and slicing with other kind of ranges as well then. Seems strange to only support slicing by RangeFull. Also, these should probably be implemented on CStr and relied on Deref to be found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OsString does the same thing, though that's intentionally an opaque type so the situation is slightly different in that slicing it is actually "impossible"...

I'm iffy on whether we should allow slicing into a CStr, just because it doesn't seem like a very common operation. It's also not necessarily clear whether you actually want to slice as_bytes() or as_bytes_with_nul(), and &cstr[..1] is impossible to implement without panicking if it doesn't cover the entire string. It also hides the cost of the strlen which is supposed to be necessary in the future... I'm not entirely opposed to it though!

This impl was meant as a conversion trait (the good old &deref/&index[..]/AsRef/.as_slice() bunch), the main advantage being that it isn't type-ambiguous. The discussion at #27729 might be a bit relevant there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Index<RangeTo> and Index<Range> can't really work, you would have to insert a 0 byte in the middle of the string. And implementing only Index<RangeFrom> also seems strange (though potentially useful).

@aturon
Copy link
Member

aturon commented Dec 31, 2015

LGTM.

Regarding the From impl, it's fine to start with just going from &CStr. The OsString case is somewhat special, since there are several different reference types that can map into an OsString.

@arcnmx
Copy link
Contributor Author

arcnmx commented Dec 31, 2015

Pushed up the non-generic From impl.

@aturon
Copy link
Member

aturon commented Dec 31, 2015

Sorry, one other thing -- can you remove the unstable markers, since they have no effect?

@arcnmx
Copy link
Contributor Author

arcnmx commented Dec 31, 2015

@aturon yeah wasn't sure about those... All the other trait impls have stable markers, should I be using those or just leave them unmarked for now?

@aturon
Copy link
Member

aturon commented Dec 31, 2015

@arcnmx Oh hm, it's possible that the stability system requires you to have some marker, even though it's effectively ignored for impls. So yes, mark as stable, version 1.7.0. Thanks!

@arcnmx
Copy link
Contributor Author

arcnmx commented Dec 31, 2015

@aturon how does that look?

@aturon
Copy link
Member

aturon commented Dec 31, 2015

Looks great, thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Dec 31, 2015

📌 Commit 53878e7 has been approved by aturon

@bors
Copy link
Contributor

bors commented Dec 31, 2015

⌛ Testing commit 53878e7 with merge b9075d6...

bors added a commit that referenced this pull request Dec 31, 2015
Are trait impls still insta-stable? Considering that this design has been around for a long time on `String` and `OsString` it probably doesn't matter much...

The `From` impl is a bit strange to me. It's stolen from `OsString` but I'm not really sure about it... `String` just impls `From<&str>` instead, would that make more sense?
@bors bors merged commit 53878e7 into rust-lang:master Dec 31, 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