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

Implemented [&]Cow<str> for SharedString #6371

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

npwoods
Copy link

@npwoods npwoods commented Sep 29, 2024

Title is self-explanatory

@CLAassistant
Copy link

CLAassistant commented Sep 29, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your PR.

I think it somehow makes sense.
Although this doesn't bring much value over shared_string.as_ref().into()

What is your use-case? Is the version that take a reference really necessary?

A small #[test] would be nice.

Comment on lines 15 to 16
#[cfg(feature = "std")]
use std::borrow::Cow;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need feature="std" if you use it from alloc:

Suggested change
#[cfg(feature = "std")]
use std::borrow::Cow;
use alloc::borrow::Cow;

then the feature="std" is also not needed later.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Learned something new. Thanks!

@ogoffart
Copy link
Member

(Note that in the std library, From<Cow<str>> for String make sense since it can move the String when the Cow is owned. But this is unfortunately not possible for SharedString.)

@npwoods
Copy link
Author

npwoods commented Oct 1, 2024

This change is clearly a very minor (borderline minuscule) improvement. Yes, it just saves an as_ref() but in a hypothetical future where there is a direct conversion from String to SharedString it would be beneficial (in a small way)

Strictly speaking, From<String> is not needed because as_str() could be use. (Yeah, of course, String is much more pervasively used than Cow<str>).

Thanks!

@ogoffart
Copy link
Member

ogoffart commented Oct 1, 2024

`One thing i was considering was to make SharedString able to hold &'static str depending on some bit tag.
eg: Improve SharedString to hold either of

  • a pointer to a shared string like currently.
  • a pointer to a static string and a length
  • be inline (small string optimization)

In this case, it would be nice if one could convert from Cow<'static, str> without making an allocation.

In order to make it possible, would it make sense to restruct the implementation to 'static

Why do we need both Cow and &Cow ?

@npwoods
Copy link
Author

npwoods commented Oct 1, 2024

In this case, it would be nice if one could convert from Cow<'static, str> without making an allocation

In that case you would not be able to use the changes I made, as (to my knowledge) there would not be a way to have both an implementation that converts from Cow<'static, str> and also a generic implementation for lifetimes over than 'static

Also, doesn't Slint rely SharedString being NUL-terminated?

Why do we need both Cow and &Cow?

We probably don't - I suppose I was just "aping" what I saw with [&]String

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.

3 participants