[PATCH] D128981: [C++20][Modules] Implement include translation.

[Iain Sandoe via Phabricator via cfe-commits]

iains marked 3 inline comments as done.
iains added inline comments.


================
Comment at: clang/include/clang/Lex/Preprocessor.h:434
+    /// Saw a 'module' identifier.
+    void handleModule(bool ATLTS) {
+      // This was the first module identifier and not preceded by any token
----------------
ChuanqiXu wrote:
> What's the meaning of `ATLTS`?
`AfterTopLevelTokenSeq` I guess it seemed a rather long name (but I do not mind to spell it out if that makes sense).



================
Comment at: clang/lib/Lex/PPDirectives.cpp:2226-2227
+
+  // FIXME: We do not have a good way to disambiguate C++ clang modules from
+  // C++ standard modules (other than use/non-use of Header Units).
+  Module *SM = SuggestedModule.getModule();
----------------
ChuanqiXu wrote:
> From what @rsmith said in https://reviews.llvm.org/D113391, it looks like the ideal direction is to use C++ clang modules and C++ standard modules together. So it looks like we couldn't disambiguate them from command line options.
Well, I think there is some more debate to have around how to solve this problem (i.e. it might be intended that clang++ modules and standard c++ modules converge but as things stand we still need to support the cases that they have different behaviour, or break existing users) 
 ... - but please let us not have that debate in this patch :-)



================
Comment at: clang/test/Modules/cxx20-include-translation.cpp:10
+// RUN: %clang_cc1 -std=c++20 -xc++-user-header h4.h -emit-header-unit -o h4.pcm
+
+// RUN: %clang_cc1 -std=c++20 Xlate.cpp -emit-module-interface -o Xlate.pcm \
----------------
ChuanqiXu wrote:
> Maybe we need an example `h5.h` which is not pre-compiled  as a header unit and it would be handled correctly.
I am not sure if you mean that the un-converted header would be included in the GMF or in the module purview - IMO we already have test-cases that cover this, but I do not mind adding something extra if there's a missing case?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128981