Also, now that I look at it more, I'm confused what the purpose of the
first hunk is? How does allowing the second index to run off the end
of the string help us here?
The first hunk is dead code. Here is the full context:
if (v->end < 0) {
// => v->end < 0
v->end += zlen;
// => v->end < zlen
if (v->end < 0) {
v->end = 0;
} else if (v->end >= zlen) { // => never true
v->end = zlen;
} else {#ifdef MULTIBYTE_SUPPORT
if (isset(MULTIBYTE)) {
// => v->end < zlen
v->end += MB_METACHARLEN(z + v->end);
// Here v->end could become greater than zlen if MB_METACHARLEN(...) returns a value greater than 1
} else {
v->end++;
}
#else
v->end++;
#endif
}
}
else if (v->end > zlen)
v->end = zlen;
One thing that stands out when reviewing it is that the second hunk is
adding an "if" between an existing "if" and "else if", which feels a
little weird.
Because of the last comment in the code above, I find it's better to always check for v->end > zlen. It makes it much more obvious that v->end will never be greater than zlen at the end of the chunk of code. I assume (but I haven't checked) that MB_METACHARLEN() never leads v-end to overtake zlen. So technically keeping the else if would also work and would sometimes avoid one extra check. If you feel strongly about it, I can re-add it.
Philippe