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

[Iain Sandoe via Phabricator via cfe-commits]

iains 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();
----------------
ChuanqiXu wrote:
> 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.
> 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`.

OK, I've made this private and updated the comment to note that this is a helper for 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.

Is this a rule?

I think that the name `ActOnStartOfHeaderUnit()` says exactly what we are doing, of course, at some time we might need an `ActOnEndOfHeaderUnit()` - but we should not add an empty function for that reason.

(this is not a sticking point; if consensus is that the name is confusing, I will change it).



================
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;
   }
----------------
ChuanqiXu wrote:
> How about
> ```
> return InternalLinkage ? M->getTopLevelModule() : nullptr; 
> ```
that would alter the behaviour of the existing code.

getTopLevelModule() will return the ultimate parent:
```
  const Module *Result = this;
  while (Result->Parent)
    Result = Result->Parent;
```
where the existing code only returns the immediate parent (perhaps that is unintended, but it should be fixed separately if so).



================
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)
----------------
ChuanqiXu wrote:
> The comment is not accurate. `header unit import` is pre-processor too. http://eel.is/c++draft/cpp.import
> The comment is not accurate. `header unit import` is pre-processor too. 

the pre-processor 'import' is a pre-processor directive. 

https://eel.is/c++draft/cpp.pre#1

I amended to clarify that we cannot import a named module in this position - only a header unit.


================
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.
----------------
ChuanqiXu wrote:
> I would feel better if we add an assertion below to assert `ModuleScopes.back().Module->isGlobalModule()` is true only if Mod is Header Unit.
> 
OK, done.
I also added a check for CPlusPlus modules, since modules-ts has an implicit GMF and slightly different rules.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121095