Re: [RFC 8.3 3/3] Avoid a crash in source_cache::extract_lines

[Pedro Alves <palves@redhat.com>]

On 03/13/2019 05:20 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Can you clarify how can first_pos end up as npos?  Is that a bug in the
> Pedro> caller, or is it normal?  The documentation doesn't seem to allow for that:
> 
> Pedro>   /* Get the source text for the source file in symtab S.  FIRST_LINE
> Pedro>      and LAST_LINE are the first and last lines to return; line
> Pedro>      numbers are 1-based.  If the file cannot be read, false is
> Pedro>      returned.  Otherwise, LINES_OUT is set to the desired text.  The
> Pedro>      returned text may include ANSI terminal escapes.  */
> 
> I think you're just confusing first_pos and first_line here.
> first_pos is a local variable that's used to track the position where
> the first line starts:
> 
>   std::string::size_type first_pos = std::string::npos;
> [...]
>       if (lineno == first_line)
> 	first_pos = pos;
> 
> It can still be npos if first_line is greater than the number of lines
> in the file.

Oh, I see now, now that I actually look at the code, rather than just the
patch.  Sorry about that.  And now that I look, I admit it took me a bit
to grok the function, but I got it.

IIUC, the function can never really return false, right?  Since
get_source_lines already validates input.  If you made extract_lines
return std::string instead of using an output parameter, then you
could conveniently write:

	  if (first_pos == std::string::npos)
	    return {};

for this case, which might be a little clearer than the
resulting "npos - npos" with your patch.

Anyhow, not that important.

Patch LGTM.

Thanks,
Pedro Alves