On Wed, 2024-11-27 at 14:56 +0100, Richard Biener wrote:
> On Sun, Nov 3, 2024 at 11:28 PM Lewis Hyatt <[email protected]> wrote:
> >
> > Several of the selftests in diagnostic-show-locus.cc and input.cc
> > are
> > sensitive to linemap internals. Adjust them here so they will
> > support 64-bit
> > location_t if configured.
> >
> > Likewise, handle 64-bit location_t in the support for
> > -fdump-internal-locations. As was done with the analyzer, convert
> > to
> > (unsigned long long) explicitly so that 32- and 64-bit can be
> > handled with
> > the same printf formats.
>
> I was hoping David would have a look here. Absent from comments from
> him
> this is OK when all else is approved and after giving him another
> week.
Mostly looks good, but I have a couple of questions below...
>
> What's missing review now? I've lost track ...
>
> Thanks,
> Richard.
>
> > gcc/ChangeLog:
> >
> > * diagnostic-show-locus.cc
> > (test_one_liner_fixit_validation_adhoc_locations): Adapt so
> > it can
> > effectively test 7-bit ranges instead of 5-bit ranges.
> > (test_one_liner_fixit_validation_adhoc_locations_utf8):
> > Likewise.
> > * input.cc (get_end_location): Adjust types to support 64-
> > bit
> > location_t.
> > (write_digit_row): Likewise.
> > (dump_location_range): Likewise.
> > (dump_location_info): Likewise.
> > (class line_table_case): Likewise.
> > (test_accessing_ordinary_linemaps): Replace some hard-coded
> > constants with the values defined in line-map.h.
> > (for_each_line_table_case): Likewise.
> > ---
> > gcc/diagnostic-show-locus.cc | 128 +++++++++++++++++++++++++++++--
> > ----
> > gcc/input.cc | 100 ++++++++++++++-------------
> > 2 files changed, 157 insertions(+), 71 deletions(-)
> >
[...snip...]
> > diff --git a/gcc/input.cc b/gcc/input.cc
> > index 04462ef6f5a..1629e4aeee8 100644
> > --- a/gcc/input.cc
> > +++ b/gcc/input.cc
[...snip...]
> > @@ -3865,11 +3870,11 @@ static const location_t
> > boundary_locations[] = {
> > LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES + 0x100,
> >
> > /* Values near LINE_MAP_MAX_LOCATION_WITH_COLS. */
> > - LINE_MAP_MAX_LOCATION_WITH_COLS - 0x100,
> > + LINE_MAP_MAX_LOCATION_WITH_COLS - 0x200,
> > LINE_MAP_MAX_LOCATION_WITH_COLS - 1,
> > LINE_MAP_MAX_LOCATION_WITH_COLS,
> > LINE_MAP_MAX_LOCATION_WITH_COLS + 1,
> > - LINE_MAP_MAX_LOCATION_WITH_COLS + 0x100,
> > + LINE_MAP_MAX_LOCATION_WITH_COLS + 0x200,
> > };
I see that this updates the offsets from 0x100 to 0x200 for the
_WITH_COLS case, but doesn't for the _WITH_PACKED_RANGES case.
What's the reasoning here?
In theory we can simply add new entries to boundary_locations to get
more test coverage, but I don't know the extent to which this part of
the selftests is slowing builds down on the slower configurations; the
selftests are meant to be fast to run.
> >
> > /* Run TESTCASE multiple times, once for each case in our test
> > matrix. */
> > @@ -3884,10 +3889,9 @@ for_each_line_table_case (void (*testcase)
> > (const line_table_case &))
> >
> > /* Run all tests with:
> > (a) line_table->default_range_bits == 0, and
> > - (b) line_table->default_range_bits == 5. */
> > - int num_cases_tested = 0;
> > - for (int default_range_bits = 0; default_range_bits <= 5;
> > - default_range_bits += 5)
> > + (b) line_table->default_range_bits ==
> > line_map_suggested_range_bits. */
> > +
> > + for (int default_range_bits: {0, line_map_suggested_range_bits})
> > {
> > /* ...and use each of the "interesting" location values as
> > the starting location within line_table. */
> > @@ -3895,15 +3899,9 @@ for_each_line_table_case (void (*testcase)
> > (const line_table_case &))
> > for (int loc_idx = 0; loc_idx < num_boundary_locations;
> > loc_idx++)
> > {
> > line_table_case c (default_range_bits,
> > boundary_locations[loc_idx]);
> > -
> > testcase (c);
> > -
> > - num_cases_tested++;
> > }
> > }
I see that this eliminates the tracking of num_cases_tested and the
assert on it below. Was this deliberate, or was the removal meant to
be a temporary thing whilst developing the patch kit?
> > -
> > - /* Verify that we fully covered the test matrix. */
> > - ASSERT_EQ (num_cases_tested, 2 * 12);
> > }
> >
> > /* Verify that when presented with a consecutive pair of locations
> > with
>
Otherwise looks good to me.
Thanks
Dave
