Re: [PATCH v4 3/8] diagnostics: Refactor class file_cache_slot

[David Malcolm <dmalcolm@redhat.com>]

On Wed, 2023-08-09 at 18:14 -0400, Lewis Hyatt wrote:
> Class file_cache_slot in input.cc is used to query specific lines of source
> code from a file when needed by diagnostics infrastructure. This will be
> extended in a subsequent patch to support obtaining the source code from
> in-memory generated buffers rather than from a file. The present patch
> refactors class file_cache_slot, putting most of the logic into a new base
> class cache_data_source, in preparation for reusing that code in the next
> patch. There is no change in functionality yet.
> 
> gcc/ChangeLog:
> 
>         * input.cc (class file_cache_slot): Refactor functionality into a
>         new base class...
>         (class cache_data_source): ...here.
>         (file_cache::forcibly_evict_file): Adapt for refactoring.
>         (file_cache_slot::evict): Renamed to...
>         (file_cache_slot::reset): ...this, and partially refactored into
>         base class...
>         (cache_data_source::reset): ...here.
>         (file_cache_slot::get_full_file_content): Moved into base class...
>         (cache_data_source::get_full_file_content): ...here.
>         (file_cache_slot::create): Adapt for refactoring.
>         (file_cache_slot::file_cache_slot): Refactor partially into...
>         (cache_data_source::cache_data_source): ...here.
>         (file_cache_slot::~file_cache_slot): Refactor partially into...
>         (cache_data_source::~cache_data_source): ...here.
>         (file_cache_slot::needs_read_p): Remove.
>         (file_cache_slot::needs_grow_p): Remove.
>         (file_cache_slot::maybe_grow): Adapt for refactoring.
>         (file_cache_slot::read_data): Refactored, along with...
>         (file_cache_slot::maybe_read_data): this, into...
>         (file_cache_slot::get_more_data): ...here.
>         (find_end_of_line): Change interface to take a pair of pointers,
>         rather than a pointer + length.
>         (file_cache_slot::get_next_line): Refactored into...
>         (cache_data_source::get_next_line): ...here.
>         (file_cache_slot::goto_next_line): Refactored into...
>         (cache_data_source::goto_next_line): ...here.
>         (file_cache_slot::read_line_num): Refactored into...
>         (cache_data_source::read_line_num): ...here.
>         (location_get_source_line): Fix const-correctness as necessitated by
>         new interface.
> ---
>  gcc/input.cc | 513 +++++++++++++++++++++++----------------------------
>  1 file changed, 235 insertions(+), 278 deletions(-)
> 

I confess I had to reread both this and patch 4/8 to make sense of
this; this is probably one of those cases where it's harder to read in
patch form than as source, but I think I now understand the new
implementation.

Did you try testing this with valgrind (e.g. "make selftest-valgrind")?

I don't think we have any selftest coverage for "\r" in the line-break
handling; that would be good to add.

This patch is OK for trunk once the rest of the kit is approved.

Thanks
Dave