Re: Fix pressing down in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

Pedro Alves
2019-03-15T13:56:45Z

On 03/15/2019 01:36 PM, Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Fri, 15 Mar 2019 12:34:34 +0000
>>
>> On 03/12/2019 04:44 PM, Eli Zaretskii wrote:
>>> 1. The first patch fixes the problem noticed by Pedro: pressing DOWN
>>> arrow in the command window doesn't scroll the source window. This is
>>> because we don't initialize the s->nlines field, and then
>>> tui_vertical_source_scroll thinks we are off the chart. This fixes
>>> that:
>>>
>>> --- gdb/source-cache.c~4 2019-03-10 08:34:47.422752400 +0200
>>> +++ gdb/source-cache.c 2019-03-12 11:50:15.094147600 +0200
>>> @@ -194,6 +194,12 @@ source_cache::get_source_lines (struct s
>>> std::ifstream input (fullname);
>>> if (input.is_open ())
>>> {
>>> + if (s->line_charpos == 0)
>>> + {
>>> + scoped_fd desc = open_source_file (s);
>>> + if (desc.get () >= 0)
>>> + find_source_lines (s, desc.get ());
>>
>> I think this should return false if open_source_file fails?
>
> How could it fail if input.is_open returns non-zero? Are you thinking
> about some race, whereby something deletes the file after the
> constructor for 'input' returns?

Yes, something like that. The possibility is small, but non-zero
that it could fail. And if it does, we end up with s->nlines
uninitialized again.

>
>> Otherwise this LGTM. I see get_plain_source_lines has similar
>> code, so my immediate thought was to move that to a helper function,
>> but there's a difference that makes that unviable -- get_plain_source_lines
>> always wants to open the source file first, while here we can avoid
>> it unless line_charpos is 0.
>
> Right. Also, I originally hoped std::ifstream will have a way to get
> at the underlying file descriptor, but no such luck, AFAICT.

Yeah, I believe GNU has a non-standard way for that, but it's not portable.
(And not worth the bother to #ifdef here for that, IMHO.

Thanks,
Pedro Alves