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

[Pedro Alves <palves@redhat.com>]

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