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