throw ArgumentError when accessing an invalid UTF-8 indice, rather th…

[tkelman]

…an attempting to adjust it. fix #7811

cc @vtjnash (any changes to make here? it's your branch, i'm just turning it into a PR) @ScottPJones

[ScottPJones]

👍

[vtjnash]

I had just forotten about this. It should be fine to merge

[tkelman]

I'm not really qualified to hit merge on anything unicode-related, could someone else review and merge if you agree with this change?

[nalimilan]

Doesn't look like you're catching all invalid indices here, are you? Shouldn't this be i > endof(s)?

julia> s = "café"
"café"

julia> nextind(s,endof(s))
6

julia> s[5]
ERROR: ArgumentError: invalid UTF-8 character index
 in next at ./utf8.jl:80
 in getindex at string.jl:62

[nalimilan]

From what I can tell it's OK (but I may have missed details). Regarding my remarks about bounds checking, do as you prefer!

[ScottPJones]

Why doesn't this also do the bounds checking that was added for search? Is it handled in the other rsearch method?
I think the bounds check needs to be done before you return 0 (for c < 0x80)

[ScottPJones]

Anybody concerned about the issue in rsearch?

[ScottPJones]

@tkelman 😀 How about addressing the issues I raised reviewing your PR? Seriously, it would be nice to get these sorts of fixes in...

[tkelman]

I don't actually know anything about what this code is doing. I just wanted to ping @vtjnash by opening this so he wouldn't forget about it.

[ScottPJones]

Oh, sorry, I saw the "Author" tag, and had forgotten that you'd just put his branch into a PR...

[jiahao]

@tkelman 😀 How about addressing the issues I raised reviewing your PR?

@ScottPJones kindly respect others' time by not pinging them frivolously. This is a borderline violation of our community standards, viz.

Be aware that most community members contribute on a voluntary basis, so ideas and bug reports are ok, but demands are not.

[vtjnash]

Anybody concerned about the issue in rsearch?

yes, but i was not prepared when making this PR to extend it further and test rsearch as well. that should be a separate commit, and perhaps a separate PR anyways.

[ScottPJones]

@jiahao That was NOT a frivolous ping. It was simply an honest mistake, pinging tkelman instead of vtjnash, because of the line at the top: "tkelman wants to merge...", which I'd already said I was sorry for. Your ping could be considered frivolous, OTOH...
@vtjnash OK, that makes sense to do it in another PR, but you didn't address the other issue I raised:

I think the bounds check needs to be done before you return 0 (for c < 0x80)

[vtjnash]

@ScottPJones it is counterproductive to try to point blame back at another volunteer in the community.

I think the bounds check needs to be done before you return 0 (for c < 0x80)

Is this in the context of rsearch? I believe I covered this case for search.

i'm definitely trying to get back to the PR, but still trying to get caught back up on everything else right now.

[ScottPJones]

@vtjnash I wasn't trying to "point blame". I was a bit taken aback by being told it was a "frivolous" ping, when pinging tkelman... which was an honest mistake, because it was he who brought me into this thread in the first place! I just wanted him to remember this issue, since nobody had responded to my comment here, and he was in contact with me constantly on a couple other threads, reviewing the Unicode PRs I've been trying to get in. Hence the smiley face. If I'd known that this PR was really you, I wouldn't have said anything, assuming you could be too busy to respond yet.

Looking again at it (and not from on my phone), that is rsearch, you have taken care of it for search.
rsearch will need to be fixed another day.

[tkelman]

Arguing about pinging back and forth is indeed unproductive. Given the new unicode error handling framework from #11573, the patch in this PR would need to be adapted to fit into it to resolve the conflict.

[vtjnash]

Given the new unicode error handling framework from #11573, the patch in this PR would need to be adapted to fit into it to resolve the conflict.

I've pushed an update for that.

Why doesn't this also do the bounds checking that was added for search? Is it handled in the other rsearch method?

afaict, none of the rsearch methods do this. also, as a breadcrumb, i have not attempted to add validation the endpoint index passed to a few of these methods (getindex(s::UTF8String, r::UnitRange{Int}) or SubString)

[ScottPJones]

Ok, good point about the other places lacking validation. 👍 to this change

[ScottPJones]

I think I see a problem with part of this change, in next(), where it now simply gives an index error if it is not at a valid UTF-8 starting character. The problem is, since strings are not validated in Julia, that the index might have hit a Latin1 character, for example, and then get an invalid index error, instead of skipping the bad byte, and returning a 0xfffd replacement character, as it does elsewhere, [or give an invalid character error].