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