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

Pedro Alves
2019-03-13T18:06:18Z

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