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

feature request: Add impl TryFrom<Arc<str>> for Regex #1257

Open
al8n opened this issue Apr 4, 2025 · 7 comments
Open

feature request: Add impl TryFrom<Arc<str>> for Regex #1257

al8n opened this issue Apr 4, 2025 · 7 comments

Comments

@al8n
Copy link

al8n commented Apr 4, 2025

Hi, thanks for the amazing crate!

As the internal pattern is an Arc<str>, is it possible to add impl From<Regex> for Arc<str> and impl From<&Regex> for Arc<str> implementation? It is useful when doing conversions between types.

@BurntSushi
Copy link
Member

Well, it certainly can't be From<Arc<str>>. I would be open to a impl TryFrom<Arc<str>> for Regex though, since that's consistent with existing TryFrom<&str> and TryFrom<String> impls.

I don't see why there should be an impl From<&Regex> for Arc<str> though. There is already a Regex::as_str, and implementing From<&Regex> for the various string types seems a little weird to me. I'm unclear on the use case.

As the internal pattern is an Arc<str>

The internal representation should have absolutely nothing to do with this. If you're mentioning this because you think it makes the conversion cheaper, then that is almost certainly not true in any meaningful sense.

@al8n
Copy link
Author

al8n commented Apr 4, 2025

Well, it certainly can't be From<Arc<str>>. I would be open to a impl TryFrom<Arc<str>> for Regex though, since that's consistent with existing TryFrom<&str> and TryFrom<String> impls.

I don't see why there should be an impl From<&Regex> for Arc<str> though. There is already a Regex::as_str, and implementing From<&Regex> for the various string types seems a little weird to me. I'm unclear on the use case.

As the internal pattern is an Arc<str>

The internal representation should have absolutely nothing to do with this. If you're mentioning this because you think it makes the conversion cheaper, then that is almost certainly not true in any meaningful sense.

The use case for impl From<Regex> for Arc<str> and impl From<&Regex> for Arc<str> is FFI, indeed, the current API, users can get the &str, but it will require another allocation to construct a new Arc<str> for FFI

@BurntSushi
Copy link
Member

That isn't a compelling enough motivation on its own IMO. That impl would require regex to always internally have an Arc<str>, and this may not always be true. Indeed, it hasn't always been true.

@al8n
Copy link
Author

al8n commented Apr 5, 2025

That isn't a compelling enough motivation on its own IMO. That impl would require regex to always internally have an Arc<str>, and this may not always be true. Indeed, it hasn't always been true.

Got it, yes, the internal may change in the future.

@al8n al8n closed this as completed Apr 5, 2025
@BurntSushi
Copy link
Member

BurntSushi commented Apr 5, 2025

I don't get why you closed this? I did say that adding impl TryFrom<Arc<str>> for Regex should be totally fine.

@BurntSushi BurntSushi reopened this Apr 5, 2025
@al8n
Copy link
Author

al8n commented Apr 5, 2025

I don't get why you closed this? I did say that adding impl TryFrom<Arc<str>> for Regex should be totally fine.

I closed this because impl From<Regex> for Arc<str> will not be added. May be I should change the title of this issue to impl TryFrom<Arc<str>> for Regex?

@BurntSushi BurntSushi changed the title feature request: Add impl From<Regex> for Arc<str> feature request: Add impl TryFrom<Arc<str>> for Regex Apr 5, 2025
@BurntSushi
Copy link
Member

Ah I see. I think a misread that your request wanted a From<Arc<str>> for Regex impl. My bad. But I think that is a valid thing to want, so I changed the title to reflect that. But I get why you closed this now. Sorry for the misunderstanding!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants