Skip to content

std::ascii: Provide a copyless [Ascii] -> str method. #10333

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 1 commit into from
Nov 8, 2013

Conversation

huonw
Copy link
Member

@huonw huonw commented Nov 7, 2013

This renames to_str_ascii to as_str_ascii and makes it non-copying,
which is possible now that strings no longer have a hidden extra
byte/null terminator.

Fixes #6120.

@Kimundi
Copy link
Member

Kimundi commented Nov 7, 2013

Replacing to_str_ascii with into_str is strange, because into_str is implemented inconsistent compared to to_str:

"foo".as_ascii().to_str() == "[f, o, o]" // because of generic impl of ToStr for &[T] with T: ToStr
"foo".as_ascii().into_str() == "foo" // Because into_str is implemented for &[Ascii]

I originally added the to_str_ascii method because of this issue: A &[Ascii] is both a ASCII string and a vector of ASCII codepoints, so there are two ways to convert it into a string.

The true fix here would be to have an str_ascii unsized type as a newtype wrapper around [Ascii], just like in theory str is a newtype wrapper around [u8] today.

.as_ascii() would then return a &str_ascii instead of a &[Ascii], and str_ascii could implement both ToStr and IntoStr as returning "foo" in the example above.

But because that's not possible yet, I'd keep to_str_ascii for now.

@huonw
Copy link
Member Author

huonw commented Nov 7, 2013

Replacing to_str_ascii with into_str is strange, because into_str is implemented inconsistent compared to to_str:

There is no mention of to_str in this PR. Replacing to_str_ascii with into_str is because it's converting (~[Ascii]) -> ~str; the old code using to_str_ascii would borrow that to &[Ascii] and allocate itself a new ~str. (into_str and to_str_ascii have the same format, i.e. "foo"... I don't see why being inconsistent with to_str is directly relevant?)

But because that's not possible yet, I'd keep to_str_ascii for now.

This is actually keeping it, just converting it to be &[Ascii] -> &str rather than &[Ascii] -> ~str (i.e. not doing a malloc internally) and changing its name to match the convention.

(I agree that having a proper ascii string is the "correct" fix.)

@Kimundi
Copy link
Member

Kimundi commented Nov 7, 2013

Sorry about the confusion, I misunderstood a few things. :)

What I wanted to say is this: Giving &[Ascii] a as_str_ascii method is perfectly fine, but seeing you replacing the old to_str_ascii calls with into_str calls, I reached the conclusion that ToStrConsume should not be implemented on ~[Ascii] at all because it leads to an inconsistency with the output of to_str(). Instead, there should be an into_str_ascii method.

However, seeing how the proper fix of having an ascii string type would allow to consistently implement both ToStr and ToStrConsume, I now think that keeping the current situation is fine too.

(Also, I think I introduced the ToStrConsume trait for use on a ~[Ascii] to begin with, and just forgot, so you really have all right to blame me for confusing you ;))

@Kimundi
Copy link
Member

Kimundi commented Nov 7, 2013

Ah, one more thing: While you're changing stuff in the ascii module anyway, could you remove the doc comment wording that talks about a null terminator? As neither str nor [Ascii] use such a thing now, having the docs talk about them is just confusing and wrong.

This renames to_str_ascii to as_str_ascii and makes it non-copying,
which is possible now that strings no longer have a hidden extra
byte/null terminator.

Fixes rust-lang#6120.
bors added a commit that referenced this pull request Nov 8, 2013
This renames to_str_ascii to as_str_ascii and makes it non-copying,
which is possible now that strings no longer have a hidden extra
byte/null terminator.

Fixes #6120.
@bors bors closed this Nov 8, 2013
@bors bors merged commit b95a8c6 into rust-lang:master Nov 8, 2013
@huonw huonw deleted the ascii branch November 25, 2013 10:56
Jarcho pushed a commit to Jarcho/rust that referenced this pull request Feb 26, 2023
…=flip1995

lintcheck: fix clap panic

clap 4.1.4 panics if `-` is used at the start of an argument:

```
$ cargo lintcheck
[…]
thread 'main' panicked at 'Argument recursive: long "--recursive" must not start with a `-`, that will be handled by the parser', /home/sam/.cargo/registry/src/github.1485827954.workers.dev-1ecc6299db9ec823/clap-4.1.4/src/builder/debug_asserts.rs:82:13
```

changelog: none
<!-- changelog_checked -->
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.

Add a method to convert a vector of Ascii to a string slice without copying
4 participants