On Tue, 2025-05-27 at 17:21 -0400, Patrick Palka wrote:
>
> On Tue, 27 May 2025, Patrick Palka wrote:
>
> > On Tue, 27 May 2025, David Malcolm wrote:
> >
> > > On Fri, 2025-05-23 at 16:58 -0400, Jason Merrill wrote:
> > > > On 4/14/25 9:57 AM, Jason Merrill wrote:
> > > > > On 1/9/25 10:00 PM, Jason Merrill wrote:
> > > > > > Tested x86_64-pc-linux-gnu. Is the diagnostic.h change OK
> > > > > > for
> > > > > > trunk?
> > > > >
> > > > > Ping?
> > > >
> > > > Ping.
> > >
> > > Sorry for the delay in responding; comments below...
> > >
> > > >
> > > > > > -- 8< --
> > > > > >
> > > > > > To respect the #pragma diagnostic lines in libstdc++
> > > > > > headers when
> > > > > > compiling
> > > > > > with module std, we need to represent them in the module.
> > > > > >
> > > > > > I think it's reasonable to make module_state a friend of
> > > > > > diagnostic_option_classifier to allow direct access to the
> > > > > > data.
> > > > > > This
> > > > > > is a
> > > > > > different approach from how Jakub made PCH streaming
> > > > > > members of
> > > > > > diagnostic_option_classifier, but it seems to me that
> > > > > > modules
> > > > > > handling
> > > > > > belongs in module.cc.
> > >
> > > Putting it in module.cc looks good to me, though perhaps it
> > > should be
> > > just a friend of diagnostic_option_classifier but not of
> > > diagnostic_context? Could the functions take a
> > > diagnostic_option_classifier rather than a diagnostic_context?
> > > diagnostic_context is something of a "big blob" of a class.
> > >
> > > [...snip...]
> > >
> > > > > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > > > > > index 78fb21dc22f..49c9c092163 100644
> > > > > > --- a/gcc/cp/module.cc
> > > > > > +++ b/gcc/cp/module.cc
> > >
> > > [...snip...]
> > >
> > > > > > @@ -17637,6 +17640,78 @@ module_state::write_ordinary_maps
> > > > > > (elf_out
> > > > > > *to, range_t &info,
> > > > > > dump.outdent ();
> > > > > > }
> > > > > > +/* Write out any #pragma GCC diagnostic info to the .dgc
> > > > > > section. */
> > > > > > +
> > > > > > +void
> > > > > > +module_state::write_diagnostic_classification (elf_out
> > > > > > *to,
> > > > > > + diagnostic_context *dc,
> > > > > > + unsigned *crc_p)
> > > > > > +{
> > > > > > + auto &changes = dc-
> > > > > > >m_option_classifier.m_classification_history;
> > > > > > +
> > > > > > + dump () && dump ("Writing diagnostic change locations");
> > > > > > + dump.indent ();
> > > > > > +
> > > > > > + bytes_out sec (to);
> > > > > > + if (sec.streaming_p ())
> > > > > > + sec.begin ();
> > > > > > +
> > > > > > + unsigned len = changes.length ();
> > > > > > + dump () && dump ("Diagnostic changes: %u", len);
> > > > > > + if (sec.streaming_p ())
> > > > > > + sec.u (len);
> > > > > > +
> > > > > > + for (const auto &c: changes)
> > > > > > + {
> > > > > > + write_location (sec, c.location);
> > > > > > + if (sec.streaming_p ())
> > > > > > + {
> > > > > > + sec.u (c.option);
> > > > > > + sec.u (c.kind);
> > > > > > + }
> > > > > > + }
> > >
> > > I confess I don't fully understand the module code yet - in
> > > particular
> > > the streaming vs non-streaming distinction. What are the "if
> > > (sec.streaming_p ())" guards doing here? It looks it can be
> > > false if
> > > the param "elf_out *to" is null (can that happen?), and if it's
> > > false,
> > > then this function essentially becomes a no-op. Is that what we
> > > want?
> >
> > When streaming_p is false then we're not serializing, we're just
> > doing
> > dependency analysis, the same walking code is used for both. The
> > trees_out
> > class definition has the following comment:
> >
> > /* The walk is used for three similar purposes:
> >
> > 1. The initial scan for dependencies.
> > 2. Once dependencies have been found, ordering them.
> > 3. Writing dependencies to file (streaming_p).
> >
> > For cases where it matters, these accessers can be used to
> > determine
> > which state we're in. */
> >
> > But it seems we don't need to check it here, since streaming_p will
> > always
> > be true at the current call sites of
> > write_diagnostic_classification?
>
> Never mind, streaming_p is clearly false for the first call.
Thanks for the clarifications
Dave
