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