[PATCH] D121097: [C++20][Modules][HU 3/5] Emit module macros for header units.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Maybe it's worth to add a test from: http://eel.is/c++draft/cpp.import#8



================
Comment at: clang/include/clang/Serialization/ASTWriter.h:127-128
 
+  /// The module is a header unit.
+  bool IsHeaderUnit = false;
+
----------------
I think the member is redundant. I thought we could use `WritingModule->isHeaderUnit()` to replace it.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:2287-2288
 /// preprocessor.
-void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) {
+void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule,
+                                  Module *Mod) {
   uint64_t MacroOffsetsBase = Stream.GetCurrentBitNo();
----------------
We could replace the use of `Mod` with WritingModule too. (If I don't misread the code)


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:2358-2378
+    bool EmittedModuleMacros = false;
+    if (IsHeaderUnit) {
+      // This is for the main TU when it is a C++20 header unit.
+      // We preserve the final state of defined macros, and we do not emit ones
+      // that are undefined.
+      if (!MD || shouldIgnoreMacro(MD, IsModule, PP) ||
+          MD->getKind() == MacroDirective::MD_Undefine)
----------------
Is it possible to merge the implementation with the following for PCH? It looks like there are some redundancies.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:2360-2362
+      // This is for the main TU when it is a C++20 header unit.
+      // We preserve the final state of defined macros, and we do not emit ones
+      // that are undefined.
----------------
How about to add comments to show the **different logic** of emitting macros for header unit and PCH? I guess other readers might be confusing too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121097