[PATCH] D119823: [Modules] Add module structure output to -module-file-info.

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added inline comments.


================
Comment at: clang/test/Modules/module-file-info-cxx20.cpp:26
+
+#if TU == 1
+
----------------
iains wrote:
> urnathan wrote:
> > urnathan wrote:
> > > ChuanqiXu wrote:
> > > > iains wrote:
> > > > > ChuanqiXu wrote:
> > > > > > According to [[ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p1857r3.html | P1857R3 ]], it might not be good to add macro declarative before module declaration. Although clang didn't implement it and there are old test case uses this style, I think it might be better to split into files. @urnathan how do you think about this?
> > > > > I see this in the Tony tables, but OTOH I cannot exactly see where the wording forbids it - in fact it permits preprocesssor directives before the import (including in the example in 5.7)... perhaps I'm misreading.
> > > > > 
> > > > > This way of writing the test cases does make them much easier to manage (and for a reader to see the intent) - of course, if we should split into files - then that is what we should do.
> > > > > 
> > > > I think the grammar in [[ http://eel.is/c++draft/cpp | [cpp.pre] ]] forbid it.
> > > > 
> > > > First, the file could only be:
> > > > ```
> > > > preprocessing-file:
> > > > group_opt
> > > > module-file
> > > > ```
> > > > 
> > > > Then it is clear that we couldn't put macro declarative before module declaration. I agree the current style is easier and more convenient. In fact, I prefer it too. But I think we would better follow it since it is standard. Otherwise, it would be more work in later maintenance stage.
> > > ChuanqiXu is correct about preprocessor directives not being allowed before the initial module decl, but I don;t think compilers implement that.  There are a couple of issues.
> > > 
> > > a) some users have a need to have #pragma charset-or-similar before any tokens
> > > b) forced headers
> > > 
> > > I do find the #if use here somewhat confusing.  A bunch of cxx20-file-info-[1-n].cpp might be more straightforwards?  (It does seem that directory is using cxx20 as a prefix rather than suffix btw.)
> > or maybe splitting this up make it harder to understand the tests as a whole.  I don;t think it terribly importand.  If we do ban pre-module directives, we can always split it then,
> OK ( actually, I find it easier to grok the test case intent if the various required inputs are built in order, in the single file - but that's the nature of preferences :) )
> 
> I suppose that (a) and (b) mean that we cannot reasonably diagnose this?
> 
> Probably some of the files (e.g. ones making dummy modules to be consumed by the actual tests) could be in 'Inputs'.  Will rework this accordingly.

> I suppose that (a) and (b) mean that we cannot reasonably diagnose this?

The #pragma item came up in stdization and IIRC there was some handwaving that such a pragma is not actually in the source text -- because it's telling you something about the encoding.  One could probably make a similar argument about forced headers -- they're implementation detail (that the user might be able to control).

Anyway, IMHO this is orthogonal, and we might even be able to deploy something like:
```
#pragma GCC system_header
#look ma! random directive
module;
```
if we do start being pickier.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119823/new/

https://reviews.llvm.org/D119823