-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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] Optional start
and length
arg for String._strref_dangerous
#2719
Conversation
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
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.
Thanks for the patch! @JoeLoser Do you have thoughts on this?
Co-authored-by: Laszlo Kindrat <laszlokindrat@gmail.com> Signed-off-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Signed-off-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Signed-off-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Signed-off-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
In general, use of |
Yeah, although I think this could be useful in the meanwhile, and low risk since it's a hidden API anyway. @fknfilewalker Could you please rebase after the next nightly, and change this patch to update the potential uses in |
Signed-off-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
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.
Thank you! I have a few more requests, but LGTM otherwise!
Signed-off-by: Lukas Lipp <15105596+fknfilewalker@users.noreply.github.com>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Signed-off-by: Lukas Lipp <llipp@cg.tuwien.ac.at>
Also, can you please update |
For now I only checked string.mojo and there are not many places where it is really helpful and since you want to replace it with |
Is it really only one place where it's useful? I would have expected more. If you find one or two more places, I think we should land it, otherwise you're probably right, it's not pulling its weight. |
@fknfilewalker Did you find any other uses for this? If not, I think I will just close it for now, and we can revive later if it becomes relevant again. |
We now have https://github.com/modularml/mojo/blob/nightly/stdlib/src/utils/string_slice.mojo so maybe this pull request is not needed anymore? |
@fknfilewalker Thanks for your work on this so far. Regarding whether we want to move forward with this, I think at the moment this change seems a bit under-motivated. From looking at where this new functionality is used, I think every call site is an example of a place that we will eventually want to migrate to using the relatively new We really appreciate you taking the time to make this contribution, but I think we're going to hold off on accepting it. There are other valuable and worthwhile directions for improvements to the string handling code that could be made and we'd be eager to accept—if you're interested in helping out we'd love to accept those kinds of changes 🙂. For example, as mentioned, we could transition away from using |
I support this decision :) |
Normally when one wants to create a truncated StringRef, a ptr to the String together with offsets need to be used. This is annoying and dangerous and could be done more safely during the
_strref_dangerous
call. This also adds bound guards