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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
