[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
+
----------------
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,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119823