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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

LGTM with suggested changes.



================
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();
----------------
iains wrote:
> 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).
> 
I don't find the rule that there must be a pair for `ActOnStartOf*` and `ActOnEndOf*`. I just find almost all other methods does so. For the consistency problem, I would still suggest to change the name to something else.


================
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;
   }
----------------
iains wrote:
> 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).
> 
I am sure that in C++20 modules, there would be 2 levels of hierarchy at most. I am OK to fix it later (Hope we don't forget it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121095