[PATCH] D121095: [C++20][Modules][HU 1/5] Introduce header units as a module type.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:2978-2980
+  /// The parser has begun a translation unit to be compiled as a C++20
+  /// Header Unit.
+  void ActOnStartOfHeaderUnit();
----------------
>From the implementation, I think it should be called only in `ActOnStartOfTranslationUnit`. So it would be better to move this function to private section to avoid accident calls. We should add such constraint as assumption or at least comment to tell it should be called by `ActOnStartOfTranslationUnit`.

---

The name `ActOnStartOf*` implies there should be a corresponding `ActOnEndOf*`  methods. But there isn't one. So I would suggest to give another name to avoid ambiguity.


================
Comment at: clang/lib/AST/Decl.cpp:1573-1574
       InternalLinkage = isInAnonymousNamespace();
-    return InternalLinkage ? M->Parent : nullptr;
+    return InternalLinkage ? M->Kind == Module::ModuleHeaderUnit ? M : M->Parent
+                           : nullptr;
   }
----------------
How about
```
return InternalLinkage ? M->getTopLevelModule() : nullptr; 
```


================
Comment at: clang/lib/Parse/Parser.cpp:2476
     // We can only have pre-processor directives in the global module
     // fragment.  We can, however have a header unit import here.
+    if (!HeaderUnit || HeaderUnit->Kind != Module::ModuleKind::ModuleHeaderUnit)
----------------
The comment is not accurate. `header unit import` is pre-processor too. http://eel.is/c++draft/cpp.import


================
Comment at: clang/lib/Sema/SemaModule.cpp:519
+             (ModuleScopes.back().ModuleInterface ||
+              ModuleScopes.back().Module->isGlobalModule())) {
     // Re-export the module if the imported module is exported.
----------------
I would feel better if we add an assertion below to assert `ModuleScopes.back().Module->isGlobalModule()` is true only if Mod is Header Unit.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121095