ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land.
LGTM. ================ Comment at: clang/test/Modules/module-file-info-cxx20.cpp:26 + +#if TU == 1 + ---------------- urnathan wrote: > 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. OK, so I summarize the conclusion here is to allow use of macro declarative in test now and we could refactor in one shot once compiler implement the constraint. It is good for me as long as we get in consensus. 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
