Subject: Re: bug#78735: feature/igc: [PATCH] Reduce the size of the kbd-buffer GC root

From: Pip Cet <[email-masked]>
Date: Fri, 13 Jun 2025 13:07:50 +0000

"Helmut Eller" <[email-masked]> writes:

> On Mon, Jun 09 2025, Pip Cet wrote:
>
> [...]
>>> +static union buffered_input_event *
>>> +prev_kbd_event (union buffered_input_event *kbd_buffer,
>>> +		union buffered_input_event *ptr)
>>> +{
>>> +  return ptr == kbd_buffer ? kbd_buffer + KBD_BUFFER_SIZE - 1 : ptr - 1;
>>> +}
>>> +
>>> +static union buffered_input_event *
>>> +next_kbd_event (union buffered_input_event *kbd_buffer,
>>> +		union buffered_input_event *ptr)
>>> +{
>>> +  return ptr == kbd_buffer + KBD_BUFFER_SIZE - 1 ? kbd_buffer : ptr + 1;
>>> +}
>>
>> Just out of curiosity, is there a reason for the extra kbd_buffer
>> argument?  Since this code is replicated a few times, maybe it would be
>> better to keep next_kbd_event in keyboard.c identical to the one in
>> igc.c.  Maybe not, though...
>
> The extra argument avoids accessing the global variable; that's all.
>
>>> +  union buffered_input_event *fetch
>>> +    = prev_kbd_event (kbd_buffer, kbd_fetch_ptr);
>>> +  union buffered_input_event *store
>>> +    = next_kbd_event (kbd_buffer, kbd_store_ptr);
>>> +
>>> +  if (fetch < store)
>>> +    return scan_ambig (ss, fetch, store, closure);
>>
>> I think this will fail if kbd_store_ptr + 1 == kbd_fetch_ptr (i.e. the
>> kbd_buffer is full)?
>
> Hmm, indeed.
>
>> if (fetch < store - 1)
>>
>> should work, though, and it's not like this case is common enough to
>> worry about scanning the same event twice.
>
> Yes, that should work.

I've applied a new version of this patch, and am closing the bug.
Thanks again, Helmut!

Pip