On Fri, 2022-01-14 at 23:01 -0500, Jason Merrill wrote:
> On 1/13/22 17:30, David Malcolm wrote:
> > On Thu, 2022-01-13 at 17:08 -0500, Jason Merrill wrote:
> > > When a sequence of diagnostic messages bounces back and forth
> > > repeatedly
> > > between two includes, as with
> > >
> > > #include <map>
> > > std::map<const char*, const char*> m ("123", "456");
> > >
> > > The output is quite a bit longer than necessary because we dump
> > > the
> > > include
> > > path each time it changes. I'd think we could print the include
> > > path
> > > once
> > > for each header file, and then expect that the user can look
> > > earlier
> > > in the
> > > output if they're wondering.
> > >
> > > Tested x86_64-pc-linux-gnu, OK for trunk?
> > >
> > > gcc/ChangeLog:
> > >
> > > * diagnostic.c (includes_seen): New.
> > > (diagnostic_report_current_module): Use it.
> > > ---
> > > gcc/diagnostic.c | 12 +++++++++++-
> > > 1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
> > > index 58139427d01..e56441a2dbf 100644
> > > --- a/gcc/diagnostic.c
> > > +++ b/gcc/diagnostic.c
> > > @@ -700,6 +700,16 @@ set_last_module (diagnostic_context
> > > *context,
> > > const line_map_ordinary *map)
> > > context->last_module = map;
> > > }
> > >
> > > +/* Only dump the "In file included from..." stack once for each
> > > file. */
> > > +
> > > +static bool
> > > +includes_seen (const line_map_ordinary *map)
> > > +{
> > > + using hset = hash_set<const line_map_ordinary *>;
> > > + static hset *set = new hset;
> > > + return set->add (map);
> > > +}
> >
> > Overall, I like the idea, but...
> >
> > - the patch works at the level of line_map_ordinary instances,
> > rather
> > than header files. There are various ways in which a single header
> > file can have multiple line maps e.g. due to very long lines, or
> > including another file, etc. I think it makes sense to do it at
> > the
> > per-file level, assuming we aren't in a horrible situation where a
> > header is being included repeatedly, with different effects. So
> > maybe
> > this ought to look at what include directive led to this map, i.e.
> > looking at the ord_map->included_from field, and having a
> > hash_set<location_t> ?
>
> Done. This version doesn't affect printing of the module import path
> yet, pending more consideration of whether we always want to identify
> the module it comes from or just the file/line is enough.
Seems like a good choice given that everyone's going to be much less
familiar with modules than with include files, for some time.
>
> > - there's no test coverage, but it's probably not feasible to write
> > DejaGnu tests for this, given the way prune.exp's prune_gcc_output
> > strips these strings. Maybe a dg directive to selectively disable
> > the
> > pertinent pruning operations in prune_gcc_output??? Gah...
> >
> > - global state is a pet peeve of mine; can the above state be put
> > inside the diagnostic_context instead? (perhaps via a pointer to
> > a
> > wrapper class to avoid requiring all users of diagnostic.h to
> > include
> > hash-set.h?).
>
> It seems that using hash_set directly doesn't break any users.
Thanks. The updated patch looks good to me.
Dave
