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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
