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

Wed Feb 16 07:01:04 PST 2022

urnathan added inline comments.

================
Comment at: clang/test/Modules/module-file-info-cxx20.cpp:26
+
+#if TU == 1
+
----------------
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.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119823