ParkMyCar / compact_str

A memory efficient string type that can store up to 24* bytes on the stack
MIT License
558 stars 41 forks source link

Rethink `new_inline()` and `from_static_str()` #322

Closed Kijewski closed 5 months ago

Kijewski commented 8 months ago

In my opinion, from_static_str() should be the default option for a user to create a const or static CompactString. This way they don't have to think about how CompactStrings actually work. Things just work.

  1. I would rename from_static_str() into const_new() (borrowing the name e.g. from tokio), to make it obvious that this is the function one should use.
  2. I would rename new_inline() into try_new_inline() and make this fallible.

What do you think? I know that this is a big change, so I would keep new_inline() with a deprecation notice around for a while.

NobodyXu commented 8 months ago
  1. I would rename from_static_str() into const_new() (borrowing the name e.g. from tokio), to make it obvious that this is the function one should use.

That seems to be a reasonable API, much better than panic on too large in new_inline().

  1. I would rename new_inline() into try_new_inline() and make this fallible.

Yeah I think that's more reasonable than implicit panic.

Users could still .unwrap() it themselves if they want to, but that's explicit.

ParkMyCar commented 8 months ago

I really like this idea, especially the concept of "This way they don't have to think about how CompactStrings actually work. Things just work."

It's a bit of bikeshedding, but I'm not sure if we need to prefix try_new_inline(...) with try_, I think we can just name the API new_inline(...). For example, String::from_utf8(...) is fallible but is not prefixed with try_.

I would propose the following API:

impl CompactString {
    pub fn new(s: impl AsRef<str>) -> Self { ... }

    pub const new_const(s: &'static str) -> Self { ... }

    /// ... include thorough documentation on why this method exists, similar to from_string_buffer()
    pub const new_inline(s: &str) -> Result<Self, InlineError> { ... }
}

pub struct InlineError {
  requested_size: usize,
}

This is a breaking change for new_inline(...), but based on semver it's okay to introduce breaking changes when going from 0.X -> 0.Y, so I would be okay with making this change