-
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] Fix String.__getitem__
#2837
Conversation
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
I think this is a duplicate of #2677, right? |
sorry I hadn't seen that PR, but that one just aborts with out of bounds (which is not "safe" behavior IMO). And also I don't understand what is meant with container_name etc. And I thought Indexer should be avoided now. I added now parametrized control if you want a debug_assert in the func. |
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
That's intentional, because we don't want to use
I fixed this in #2677 before landing it internally, and it will be included in the next nightly. Could you please check after that if this patch is still relevant, and if not, please close it. |
ok I'll wait until that helper is released,
This is unrelated to whether debug_assert is used. |
String.__getitem__
and add utils.index.normalize_idx()
String.__getitem__
Thanks for clarifying, I see what you meant now! Aborting is safer than |
As title.
String.__getitem__
has no reason to be recursive, and I think autils.index.normalize_idx()
utility is needed because I've seen wildly different normalization impls over the code and none take over and under indexing into account.