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

make_, to_, and into_ methods for in-place, cloning, and "eating" conversions, respectively #2447

Closed
SoniEx2 opened this issue May 24, 2018 · 9 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@SoniEx2
Copy link

SoniEx2 commented May 24, 2018

I would like the stdlib to follow the following rules for conversion methods:

  • make_
    • Converts in-place.
    • Returns () or Result<(), E>.
  • to_
    • Creates a new object for the conversion.
    • Returns T or Result<T, E>.
  • into_
    • May or may not actually create a new object. Consumes the current object (i.e. takes an self/mut self).
    • Returns T or Result<T, E>.

Example:

let hello = String::from("Hello World");
let hello_new = hello.to_ascii_lowercase();
assert_eq!(hello_new, "hello world");
assert_eq!(hello, "Hello World");

let mut hello = String::from("Hello World");
hello.make_ascii_lowercase();
assert_eq!(hello, "hello world");

let hello = String::from("Hello World");
let hello_moved = hello.into_ascii_lowercase();
assert_eq!(hello_moved, "hello world");

The main benefits of the into_ methods is that the compiler knows how to optimize them, as they don't necessarily hit the allocator (certainly not in the into_ascii_lowercase case), which can be nice for some things.

(Accidentally put this in the wrong repo, sorry! Thanks for having rfcs and rust repos btw because they have the same/similar length)

@shingtaklam1324
Copy link

AFAIK, this distinction already exists in std, with functions like str::make_ascii_lowercase and str::to_ascii_lowercase and str::into_string so I'm not 100 % sure on what you are trying to say here.

@SoniEx2
Copy link
Author

SoniEx2 commented May 24, 2018

I'm looking for String::into_ascii_lowercase and the like. As well as making the naming part of the official guidelines, I guess, rather than just a convention.

@shingtaklam1324
Copy link

shingtaklam1324 commented May 24, 2018

ideally, String::into_ascii_lowercase should generate the same code as String::to_ascii_lowercase().into(), so that would just be an ergonomics change. However, in most cases, only one or two of make, into and to make sense to have, so I'm not sure that it is worth it to add in all of those methods.

Regarding the guidelines, into and to are already part of the guidelines, and make occurs in method names 6 times in total. std::ascii::AsciiExt is already deprecated, so that leaves us with 4, which is str::make_lowercase, str::make_uppercase, Rc::make_mut and Arc::make_mut. It's not as pervasive as into or to, so I think they should be fine, and any new ones can be judged on a case by case basis, If you do feel that it should be added into the guidelines, then I suppose create an issue/pr on the guidelines repo

@SoniEx2
Copy link
Author

SoniEx2 commented May 24, 2018

No, into_ascii_lowercase should not be the same as to_ascii_lowercase().into(), at all.

Ideally, it should be equivalent to calling make_ascii_lowercase and moving the string as normal. But it should be usable inline (see the thing I linked in OP) so it's less ugly/more functional.

@shingtaklam1324
Copy link

shingtaklam1324 commented May 24, 2018

Ok, I see what you mean and how that could be useful for method chaining and potentially performance, but I believe the reason that std doesn't have a into_ascii_lowecase function is because there is no reason to consume the String, and convention is to use a borrow when ownership is not necessary.

@SoniEx2
Copy link
Author

SoniEx2 commented May 24, 2018

There is no reason to consume the string, but you should still be able to so the compiler can optimize it away.

It would be nice if to_ascii_lowercase could be optimized such that there are 2 impls for it, one that takes self, other that takes &self. Perhaps something along the lines of:

impl SomeDummyTrait for String {
    fn to_ascii_lowercase(self) -> String {
        self.make_ascii_lowercase();
        self
    }
}

impl SomeDummyTrait for &String {
    fn to_ascii_lowercase(self) -> String {
        self.clone().to_ascii_lowercase()
    }
}

But since we can't have that, as it would break backwards compatibility, I feel like into_ascii_lowercase is the way to go.

Perhaps future versions of the language can support syntax for doing this - checking if a value is never used again, and choosing a more optimized solution.

@Centril Centril added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Oct 14, 2018
@scottmcm
Copy link
Member

I'm going to close this here and suggest the overall convention suggestion move to updating the API guidelines, specifically https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

If you have a particular method that you think should exist, please just make a PR for it and the decision can be made there -- there's no reason to keep an issue open here about it.

@SoniEx2
Copy link
Author

SoniEx2 commented Nov 10, 2020

what about stuff like make_mut and whatnot? those aren't in the guidelines?

@scottmcm
Copy link
Member

@SoniEx2 Then you can propose an update to the guidelines in the guidelines repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

4 participants