Tracking issue for make_ascii_{upper,lower}case

[alexcrichton]

This is a tracking issue for the unstable ascii feature in the standard library. These functions have the somewhat odd naming scheme of make_* (not found elsewhere in the standard library). The utility with &mut str is also somewhat questionable as there's not a lot of support for that in the standard library.

Overall this probably just largely needs a decision.

[alexcrichton]

cc @SimonSapin

[Gankra]

Minor correction: Rc::make_unique is another example of this naming.
Unstable of course.
On Aug 13, 2015 10:09 AM, "Alex Crichton" [email protected] wrote:

cc @SimonSapin https://github.com/SimonSapin

—
Reply to this email directly or view it on GitHub
#27809 (comment).

[SimonSapin]

There should be a way to do this operation without allocating. I don’t have a strong opinion on in place with &mut str v.s. consuming and returning String (as in the now-deprecated OwnedAsciiExt trait), except that the former is sometimes slightly less convenient. (You need three statements instead of one expression.)

[Ms2ger]

I don't like the &mut approach, but that might be because I'm not yet used to it.

[nox]

I would prefer consuming the given String too.

[murarth]

The &mut str approach has the advantage that it can be used to convert an arbitrary subslice within a String. If the consuming String approach is adopted instead, the ability to do that would be lost.

[SimonSapin]

You can always iterate the &mut u8 bytes in a string and map them to upper case one by one (which is what the &mut str impl does). (Although yes, doing so is unsafe and it’s nice to have a safe wrapper for it.)

[barosl]

As for naming, something like to_ascii_uppercase_in_place might be better. It clearly states the purpose of the function, and it also has a bonus point that this function will be sorted around the more canonical to_ascii_uppercase function. The drawback is that the name is much longer, but I think it won't be much a problem as the usage of this operation might be much rarer.

[barosl]

But I also think make_ makes sense if we go the route of consuming String and returning String. In that case, we would lose a safe wrapper on slices, but that's for another decision.

[SimonSapin]

+1 for *_in_place if we keep &mut self mutating method (as opposed to self consuming methods).

[SimonSapin]

If the libs team has the bandwidth, I’d like to nominate this for discussion. To sum up, options are:

• Current semantics, maybe with a rename
• Change these methods to into_ascii_*case(self) -> Self. This may need to be a separate trait.

Both are equally general: at worst if all you have is a &mut String, you can do the mem::replace dance to use methods that consume and return an owned String.

[SimonSapin]

Also, it’s possible to build this out of tree:

for byte in &mut (bytes: Vec<u8>) {
    *byte = byte.to_ascii_lowercase()
}

for byte in unsafe { (s: String).as_mut_vec() } {
    *byte = byte.to_ascii_lowercase()
}

… though it would be nice to encapsulate the unsafety in the String case.

[alexcrichton]

🔔 This issue is now entering its cycle final comment period to be deprecated in 1.8 🔔

[SimonSapin]

What’s the rationale for not stabilizing any of the two proposed designs?

[alexcrichton]

Ah yes, to clarify we concluded that this stuck out enough and could be easily enough built on crates.io (e.g. externally) that it wasn't necessary to stabilize in libstd at this time.

[SimonSapin]

could be easily enough built on crates.io

The same can be said of the entire std::ascii module (and other things in std). I don’t understand having some of the related functionality in, but leave these couple methods out (which for the str case require unsafe.)

[alexcrichton]

Sure, but that part's already stable. The make_ convention sticks out (at least to me) and I would prefer to not stabilize this.

[SimonSapin]

I’m not a big fan of make_* either.

What about .into_ascii_lowercase(self) -> Self?

[alexcrichton]

If those methods could be merged into the AsciiExt trait itself I'd be more comfortable with that, but previously they were a separate trait.

[SimonSapin]

Alright. IIRC we had a separate trait because dynamically-sized types and methods taking self by value don’t play nice with each other. But it turns out Iterator already does this: the trick is having a default implementation with a where Self: Sized bound.

See PR #31335.

[ollie27]

What do into_ascii_{upper,lower}case offer that make_ascii_{upper,lower}case don't?

Both are equally general: at worst if all you have is a &mut String, you can do the mem::replace dance to use methods that consume and return an owned String.

Is that true? If so how do you handle &mut str and &mut [u8] without two more implementations of AsciiExt for them?

If naming is still an issue with make_* how about just uppercase_ascii() and lowercase_ascii()? "uppercase" and "lowercase" are also verbs so this fits the current naming scheme.

[aturon]

This API is going back into final comment period.

We were previously unable to reach consensus on a good convention for make_ascii_*case, and tried to make some progress by going forward with into_* variants in the meantime. However:

• This led to a dispatch regression, and
• As some have pointed out, the into_ variants are ultimately redundant once we stabilize the make_ variants.

We've always been somewhat ambivalent about ASCII support in std, and have scaled it back over time, but at this point, we're reasonably committed to some core functionality, and we'd like to settle these remaining APIs.

As such, we're going to:

• Revert the change with into_ variants as well as the introduction of additional impls to support it.
• Try FCP again for determining a convention around the make_ variants.

We had a lengthy discussion in the libs team last time about conventions. The key problem is that mutating methods are usually reasonably clear verb forms (like push), and it feels unfortunate to introduce a prefix like make_. We discussion variations around _in_place but found this to be extremely verbose. And as several people have pointed out on thread, using conversion prefixes here doesn't feel great either -- it's a mutation, not a conversion.

Perhaps we can look at some other languages for inspiration on this convention. But let's try to get it settled this cycle.

[SimonSapin]

Revert the change with into_ variants as well as the introduction of additional impls to support it.

Is #32076 what you have in mind here?

[aturon]

@SimonSapin I think you already saw, but for the record, #32314 is what we had in mind.

[aturon]

Revisiting the naming concerns, I continue to feel like the sticking point is needing a verb, which we can get either through make as today, or by treating e.g. uppercase itself as a verb (as in uppercase_ascii or ascii_uppercase). I continue to prefer the latter, and agree with @ollie27 that uppercase_ascii is the smoothest name.

[SimonSapin]

I agree that all nouns can be verbed, but since uppercase-as-a-verb is spelled the same as uppercase-as-a-noun I think that uppercase_ascii doesn’t give enough information about how it differs from to_uppercase_ascii.

[sfackler]

I agree with Simon's concerns about ambiguity. How about uppercasify_ascii? :D

But in all seriousness, make_uppercase_ascii seems like the least-bad approach I've seen.

[aturon]

@SimonSapin Ah, that's an excellent point.

Maybe the right thing to do is to stick with make_ as @sfackler is saying -- and perhaps there's a latent convention here, for cases where the word for the method is not usually used as a verb, but rather an adjective (i.e., when it's describing a property rather than an action).

[alexcrichton]

The libs team discussed this issue during triage yesterday and the conclusion was to stabilize under the names make_*. There didn't seem to be any better alternative and we concluded that this is perhaps a convention to start.