Re: [PATCH RFA] input: add get_source_text_between

[David Malcolm <dmalcolm@redhat.com>]

On Fri, 2022-11-04 at 13:06 -0400, Jason Merrill wrote:
> On 11/4/22 11:16, David Malcolm wrote:
> > On Fri, 2022-11-04 at 10:27 -0400, Jason Merrill wrote:
> > > On 11/3/22 19:06, David Malcolm wrote:
> > > > On Thu, 2022-11-03 at 15:59 -0400, Jason Merrill via Gcc-
> > > > patches
> > > > wrote:
> > 
> > [...snip...]
> > 
> > > > 
> > > > 
> > > > Do you have test coverage for this from the DejaGnu side?  If
> > > > not,
> > > > you
> > > > could add selftest coverage for this; see input.cc's
> > > > test_reading_source_line for something similar.
> > > 
> > > There is test coverage for the output of the the contract
> > > violation
> > > handler, which involves printing the result of this function.
> > 
> > Thanks.   Is this test posted somwehere?  I was looking in:
> >  
> > https://gcc.gnu.org/pipermail/gcc-patches/2022-November/604974.html
> > but I'm not seeing it.  Sorry if I'm missing something here.
> 
> The tests are in the newer message
> 
>   
> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605072.html
> 
> > Ideally we should have coverage for the three cases of:
> > (a) bails out and returns NULL
> 
> This isn't tested because it should never happen.

I'm guessing someone will run into it in the wild at some point.  With
very large files we eventually give up on tracking column numbers, and
so we get location_t values with column number == 0, and FWIW
  gcc/testsuite/gcc.dg/plugin/location_overflow_plugin.c
makes it feasible to test such cases.

But obviously that's low priority.

Dave

> 
> > (b) single-line case
> 
> contracts-tmpl-spec3.C etc.
> 
> > (c) multi-line case
> 
> contracts-multiline1.C
> 
> > > index a28abfac5ac..04d0809bfdf 100644
> > > --- a/gcc/input.cc
> > > +++ b/gcc/input.cc
> > > @@ -949,6 +949,97 @@ location_get_source_line (const char
> > > *file_path, int line)
> > >     return char_span (buffer, len);
> > >   }
> > 
> > Strings in input.cc are not always NUL-terminated, so...
> > 
> > >   
> > > +/* Return a copy of the source text between two locations.  The
> > > caller is
> > > +   responsible for freeing the return value.  */
> > 
> > ...please note in the comment that if non-NULL, the copy is NUL-
> > terminated (I checked both exit paths that can return non-NULL, and
> > they do NUL-terminate their buffers).
> > 
> > OK with that nit fixed.
> 
> Thanks, pushing this: