iains marked 2 inline comments as done.
iains added a comment.
I updated this because I am going to push the latest version of the `P1815`
patch and that depends on the lookup changes.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:2082
+ Module *DeclModule = SemaRef.getOwningModule(D);
+ if (DeclModule && !DeclModule->isModuleMapModule() &&
+ !SemaRef.isModuleUnitOfCurrentTU(DeclModule) &&
----------------
ChuanqiXu wrote:
> We should check header units here.
The point of checking module map modules was to avoid affecting clang modules
with the change in semantics.
At the moment, I am not sure why we could exclude lookup from finding decls in
an imported header unit?
================
Comment at: clang/lib/Sema/SemaLookup.cpp:2101
+ if (isVisible(SemaRef, ND)) {
+ if (SemaRef.getLangOpts().CPlusPlusModules && ND->isFromASTFile()) {
+ // The module that owns the decl is visible; However
----------------
ChuanqiXu wrote:
> Let's not use `isFromASTFile()`. It is a low level API without higher level
> semantics. I think it is good enough to check the module of ND.
lookup is very heavily used; the purpose of the isFromAST() check is to
short-circuit the more expensive checks when we know that a decl must be in the
same TU (i.e. it is not from an AST file).
If we can find a suitable inexpensive check that has better semantics, I am
fine to change this,
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145965/new/
https://reviews.llvm.org/D145965
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
