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

Please use Cow<'static, str> for configuration, rather than String. #545

Closed
maypop-the-dragon opened this issue Sep 10, 2024 · 4 comments · Fixed by #546
Closed

Please use Cow<'static, str> for configuration, rather than String. #545

maypop-the-dragon opened this issue Sep 10, 2024 · 4 comments · Fixed by #546

Comments

@maypop-the-dragon
Copy link

Please use Cow<'static, str> for configuration, rather than String.

This way, we will not have to allocate new Strings all the time even when the strings are always the same.
This will also make it possible to construct configurations in const contexts.

This will allow for cleaner code and slightly improved performance.

How it is now:

// This is a config that we will use repeatedly throughout the program.
// It must be inside the body of a function because allocating `String`s is not `const`.
let config = PrettyConfig::new()
    .indentor("\t".to_string())
    .struct_names(true);

// These all involve allocating new `String`s for the new line, indentor, and separator.
println!("{}", to_string_pretty(struct_1, config.clone()).unwrap());
println!("{}", to_string_pretty(struct_2, config.clone()).unwrap());
println!("{}", to_string_pretty(struct_3, config.clone()).unwrap());

How I imagine it could be:

// This can now be global if we want!
const CONFIG: PrettyConfig = PrettyConfig::new()
    .indentor(Cow::Borrowed("\t"))
    .struct_names(true);

// This is now cheaper and cleaner.
println!("{}", to_string_pretty(struct_1, CONFIG).unwrap());
println!("{}", to_string_pretty(struct_2, CONFIG).unwrap());
println!("{}", to_string_pretty(struct_3, CONFIG).unwrap());

P.S. This is my first GitHub issue so sorry if it's, like, bad somehow.

@juntyr
Copy link
Member

juntyr commented Sep 11, 2024

Thank you for your very well-written issue, @maypop-the-dragon!

I agree that using Cow instead of String would be more ergonomic, especially if the confit setter methods would take Into<Cow> as a parameter.

The lifetime of the Cow is of course an important question. With ‘static we’d allow the optimisation only for const config values, which should cover most cases. Dynamic values would still need to allocate, but by keeping the Cow const we’d not have to add any new lifetime bounds.

Overall, I like this idea! Do you want to open a PR yourself with this change? I’m very swamped myself atm.

@maypop-the-dragon
Copy link
Author

@juntyr, this is my first time doing this, so I might need help.
Could you at least link me to a good tutorial, and are there any guidelines I should follow?

@maypop-the-dragon
Copy link
Author

Oops! Sorry, I clicked the wrong button! Again, first time.

@Coca162
Copy link
Contributor

Coca162 commented Sep 11, 2024

Just as a note but if this is done and PrettyConfig::new is made const this would still not allow users of the library to change the Cow fields in a constant context due to the compiler assuming Cow drops are not const (rust-lang/rust#73255), and the only way to construct PrettyConfigs is via updating (and thusly dropping) the default (due to #[non_exhaustive]).

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 a pull request may close this issue.

4 participants
@maypop-the-dragon @juntyr @Coca162 and others