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

Implements thisind function #24414

Merged
merged 1 commit into from
Nov 22, 2017
Merged

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Oct 30, 2017

thisind(str::AbstractString, i::Integer) gets the largest valid string index at or before i.
Returns 0 if there is no valid string index at or before i. Returns endof(str) if i≥endof(str).

This is a proposed implementation of last part of #23765.

@bkamins
Copy link
Member Author

bkamins commented Nov 21, 2017

@StefanKarpinski @nalimilan
Any opinion if this should be merged or dropped?

@StefanKarpinski
Copy link
Member

I wasn't entirely convinced that we need this function, but as it turns out I'm working on a PR where this is necessary. The generic definition of reverseind as prevind(s, ncodeunits(s)-i+2) has a broken corner case for reverseind("", 0) which should return 1 but ends up returning 0 instead. If you define it as thisind(s, ncodeunits(s)-i+1) then not only does the definition make more intuitive sense, but also the corner case is fixed.

@StefanKarpinski StefanKarpinski merged commit 600f880 into JuliaLang:master Nov 22, 2017
@nalimilan
Copy link
Member

It's a bit late, but I wonder whether curind or currentind wouldn't have been a better name. "Current" fits better in the "previous" vs "next" terminology than "this".

@StefanKarpinski
Copy link
Member

A better name could be possible. I've ended up having to modify the behavior here a bit: in order for this to fix the reverseind corner case, it needs to return ncodeunits(s) + 1 whenever i > ncodeunits(s), which fits in with the model of there being an infinite-length code point right after the end of the string for string index arithmetic purposes.

@rapus95
Copy link
Contributor

rapus95 commented Jun 10, 2018

Coming from reading the 0.7 news, wouldn't it make sense to return nothing if no such index exists since for 0.7 a lot of the index finding functions do so? That would help people getting a consistent feeling for when we use the Union-type as "nullable"

@StefanKarpinski
Copy link
Member

That would have been worth investigating a while ago but at this point I think it's too late.

@rapus95
Copy link
Contributor

rapus95 commented Jun 11, 2018

just out of curiosity about the reasons, is it too late even though there's no official non-alpha release of a pre 1.0 versioned language that contains this function? would you even consider it a breaking change if that was changed?

Edit: Rereading my text I found it to sound a bit harsh, I don't mean it that mean!

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jun 11, 2018

All of the prevind, nextind and thisind functions always return integer, not nothing. Moreover, it seems like it works pretty well for writing consistent generic string code since, e.g. i:prevind(s, j) correctly gives an empty string when i == j which is what one wants—even in the corner case where i == j == 1. I suspect this is one of those cases where returning a nothing sentinel would make it much harder and less convenient to write correct, generic string code. It's also too late to change how all the string index arithmetic functions work. It's unclear under which circumstances thisind would return nothing since it always gives an index into the same character.

@rapus95
Copy link
Contributor

rapus95 commented Jun 12, 2018

hm in context of the iterators it makes much more sense to use 0 as sentinel since you still want valid (though maybe empty) ranges. To fix this one would have to make Range(a,b) to be empty if a or b is nothing which would add some problems i guess...
Thanks for clarifying! :)

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.

4 participants