-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[stdlib] Remove StringRef
from python binding functions
#3775
base: nightly
Are you sure you want to change the base?
[stdlib] Remove StringRef
from python binding functions
#3775
Conversation
Signed-off-by: martinvuyk <[email protected]>
Signed-off-by: martinvuyk <[email protected]>
…tringref-from-python
Signed-off-by: martinvuyk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Martin, thanks for tackling this! 🙂
Seeing all the places here where you needed to add an O: ImmutableOrigin
parameter made me want to re-evaluate our current API design for StringSlice
, to see if we can't avoid having to add so many new parameters (in this PR, and in existing usage of StringSlice
).
I'm going play around with a couple design ideas, namely variations on removing the //
from StringSlice
so that the is_mutable
parameter is no longer infer-only so that the mutability can be affixed directly as something like StringSlice[Immutable]
.
This is something we had before the language added infer-only parameters, but IMO in the case of is_mutable
parameters, making them unspecifiable has this slight usability downside.
I'll report back here once that lands, and then see how it affects this PR and move forward with it 🙂
fn as_string_slice(ref [_]self) -> StringSlice[__origin_of(self)]: | ||
fn as_string_slice( | ||
ref [_]self, | ||
) -> StringSlice[_lit_mut_cast[__origin_of(self), False].result]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question Why force the return value to be an immutable slice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because otherwise we'd need to add the equivalent of Span.get_immutable()
and do string_var.as_string_slice().get_immutable()
which is way too verbose. And I figured that to keep the scope of this PR small it might be better to just force the API to return immutable slices. I'm fine with doing it either way, WDYT?
Signed-off-by: martinvuyk <[email protected]>
Remove
StringRef
from python binding functions.CC: @ConnorGray I'd also like to deprecate
.unsafe_cstr_ptr()
in PR #3638