Re: [PATCH RESEND 1/1] p1689r5: initial support

[Ben Boeckel <ben.boeckel@kitware.com>]

On Mon, Oct 10, 2022 at 17:04:09 -0400, Jason Merrill wrote:
> On 10/4/22 11:12, Ben Boeckel wrote:
> > This patch implements support for [P1689R5][] to communicate to a build
> > system the C++20 module dependencies to build systems so that they may
> > build `.gcm` files in the proper order.
> 
> Thanks!
> 
> > Support is communicated through the following three new flags:
> > 
> > - `-fdeps-format=` specifies the format for the output. Currently named
> >    `p1689r5`.
> > 
> > - `-fdeps-file=` specifies the path to the file to write the format to.
> 
> Do you expect users to want to emit Makefile (-MM) and P1689 
> dependencies from the same compilation?

Yes, the build system wants to know what files affect scanning as well
(e.g., `#include <iostream>` is still possible and if it changes, this
operation should be performed again. The `-M` flags do this quite nicely
already :) .

> > - `-fdep-output=` specifies the `.o` that will be written for the TU
> >    that is scanned. This is required so that the build system can
> >    correlate the dependency output with the actual compilation that will
> >    occur.
> 
> The dependency machinery already needs to be able to figure out the name 
> of the output file, can't we reuse that instead of specifying it on the 
> command line?

This is how it determines the output of the compilation. Because it is
piggy-backing on the `-E` flag, the `-o` flag specifies the output of
the preprocessed source (purely a side-effect right now).

> > diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
> > index 2db1e9cbdfb..90787230a9e 100644
> > --- a/libcpp/include/cpplib.h
> > +++ b/libcpp/include/cpplib.h
> > @@ -298,6 +298,9 @@ typedef CPPCHAR_SIGNED_T cppchar_signed_t;
> >   /* Style of header dependencies to generate.  */
> >   enum cpp_deps_style { DEPS_NONE = 0, DEPS_USER, DEPS_SYSTEM };
> >   
> > +/* Format of header dependencies to generate.  */
> > +enum cpp_deps_format { DEPS_FMT_NONE = 0, DEPS_FMT_P1689R5 };
> 
> Why not add this to cpp_deps_style?

It is orthogonal. The `-M` flags and `-fdeps-*` flags are similar, but
`-fdeps-*` dependencies are fundamentally different than `-M`
dependencies. The comment does need updated though.

`-M` is about discovered dependencies: those that you find out while
doing work. `-fdep-*` is about ordering dependencies: extracting
information from file content in order to even order future work around.
It stems from the loss of embarassing parallelism when building C++20
code that uses `import`. It's isomorphic to the Fortran module
compilation ordering problem.

> > @@ -387,7 +410,7 @@ make_write_vec (const mkdeps::vec<const char *> &vec, FILE *fp,
> >      .PHONY targets for all the dependencies too.  */
> >   
> >   static void
> > -make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> > +make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax, int extra)
> 
> Instead of adding the "extra" parameter...

Hmm. Not sure why I had named this so poorly. Maybe this comes from my
initial stab at this functionality in 2019 (the format has been hammered
out in ISO C++'s SG15 since then).

> >   {
> >     const mkdeps *d = pfile->deps;
> >   
> > @@ -398,7 +421,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> >     if (d->deps.size ())
> >       {
> >         column = make_write_vec (d->targets, fp, 0, colmax, d->quote_lwm);
> > -      if (CPP_OPTION (pfile, deps.modules) && d->cmi_name)
> > +      if (extra && CPP_OPTION (pfile, deps.modules) && d->cmi_name)
> >   	column = make_write_name (d->cmi_name, fp, column, colmax);
> >         fputs (":", fp);
> >         column++;
> > @@ -412,7 +435,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> >     if (!CPP_OPTION (pfile, deps.modules))
> >       return;
> 
> ...how about checking CPP_OPTION for p1689r5 mode here?

I'll take a look at this.

> >   
> > -  if (d->modules.size ())
> > +  if (extra && d->modules.size ())
> >       {
> >         column = make_write_vec (d->targets, fp, 0, colmax, d->quote_lwm);
> >         if (d->cmi_name)
> > @@ -423,7 +446,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> >         fputs ("\n", fp);
> >       }
> >   
> > -  if (d->module_name)
> > +  if (extra && d->module_name)
> >       {
> >         if (d->cmi_name)
> >   	{
> > @@ -455,7 +478,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> >   	}
> >       }
> >     
> > -  if (d->modules.size ())
> > +  if (extra && d->modules.size ())
> >       {
> >         column = fprintf (fp, "CXX_IMPORTS +=");
> >         make_write_vec (d->modules, fp, column, colmax, 0, ".c++m");
> > @@ -468,9 +491,203 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> >   /* Really we should be opening fp here.  */
> >   
> >   void
> > -deps_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> > +deps_write (const struct cpp_reader *pfile, FILE *fp, unsigned int colmax,
> > +	    int extra)
> > +{
> > +  make_write (pfile, fp, colmax, extra);
> > +}
> > +
> > +static bool
> > +is_utf8 (const char *name)
> 
> Can we share utf8 parsing code with decode_utf8_char in pretty-print.cc?

I can look at factoring that out. I'll have to decode its logic to see
how much overlap there is.

Thanks,

--Ben