Re: [PATHC] Fix handling of reversed slices in assignstrvalue

[Mikael Magnusson <mikachu@xxxxxxxxx>]

On Mon, Apr 27, 2026 at 6:10 PM Philippe Altherr
<philippe.altherr@xxxxxxxxx> wrote:
>>
>> 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.

Thanks, that makes sense. I guess this is somewhat an argument to keep
the first hunk too, it's more of a statement that v->end is < zlen,
even if it also follows from the other statements up there, and
presumably the optimizer will also be able to figure this out anyway.
If nobody else chimes in I'll just commit the second hunk (and the
tests obviously).

-- 
Mikael Magnusson