Re: [PATCH v7 2/4] p1689r5: initial support

[Jason Merrill <jason@redhat.com>]

On 7/2/23 12:32, 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.
> 
> 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.
> 
> - `-fdeps-target=` 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.
> 
> CMake supports this format as of 17 Jun 2022 (to be part of 3.25.0)
> using an experimental feature selection (to allow for future usage
> evolution without committing to how it works today). While it remains
> experimental, docs may be found in CMake's documentation for
> experimental features.
> 
> Future work may include using this format for Fortran module
> dependencies as well, however this is still pending work.
> 
> [P1689R5]: https://isocpp.org/files/papers/P1689R5.html
> [cmake-experimental]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Help/dev/experimental.rst
> 
> TODO:
> 
> - header-unit information fields
> 
> Header units (including the standard library headers) are 100%
> unsupported right now because the `-E` mechanism wants to import their
> BMIs. A new mode (i.e., something more workable than existing `-E`
> behavior) that mocks up header units as if they were imported purely
> from their path and content would be required.
> 
> - non-utf8 paths
> 
> The current standard says that paths that are not unambiguously
> represented using UTF-8 are not supported (because these cases are rare
> and the extra complication is not worth it at this time). Future
> versions of the format might have ways of encoding non-UTF-8 paths. For
> now, this patch just doesn't support non-UTF-8 paths (ignoring the
> "unambiguously representable in UTF-8" case).
> 
> - figure out why junk gets placed at the end of the file
> 
> Sometimes it seems like the file gets a lot of `NUL` bytes appended to
> it. It happens rarely and seems to be the result of some
> `ftruncate`-style call which results in extra padding in the contents.
> Noting it here as an observation at least.

Thank you for your patience, just a few tweaks left and I think we can 
put this all in this week or next.

> diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
> index aef703f8111..141cfd60eda 100644
> --- a/libcpp/include/cpplib.h
> +++ b/libcpp/include/cpplib.h
> @@ -302,6 +302,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 };
>   
> +/* Structured format of module dependencies to generate.  */
> +enum cpp_fdeps_format { DEPS_FMT_NONE = 0, DEPS_FMT_P1689R5 };

These should be FDEPS_FMT_* or just FDEPS_*.

> @@ -395,10 +423,16 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
>    if (colmax && colmax < 34)
>      colmax = 34;
>  
> +  /* Write out C++ modules information if no other `-fdeps-format=`
> +     option is given. */
> +  cpp_fdeps_format fdeps_format = CPP_OPTION (pfile, deps.fdeps_format);
> +  bool write_make_modules_deps = fdeps_format == DEPS_FMT_NONE &&
> +    CPP_OPTION (pfile, deps.modules);

We typically format an expression like this as:

> +  bool write_make_modules_deps = (fdeps_format == FDEPS_FMT_NONE
> +                                 && CPP_OPTION (pfile, deps.modules));

> @@ -473,6 +507,117 @@ deps_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
>    make_write (pfile, fp, colmax);
>  }
>  
> +static void
> +p1689r5_write_filepath (const char *name, FILE *fp)

This and the other p1689r5 functions need more comments, at least one at 
the top explaining their purpose.

Jason