ChuanqiXu added inline comments.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:2082
+ Module *DeclModule = SemaRef.getOwningModule(D);
+ if (DeclModule && !DeclModule->isModuleMapModule() &&
+ !SemaRef.isModuleUnitOfCurrentTU(DeclModule) &&
----------------
iains wrote:
> 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?
>
> At the moment, I am not sure why we could exclude lookup from finding decls
> in an imported header unit?
We're not **excluding** decls from an imported header units. We're trying to
include the decls from the headers units. Since we've allowed decls with
internal linkage to appear to header units. We must need to allow decls with
internal linkage in header units to be found here. Otherwise, the following
example may fail:
```
// foo.h
static int a = 43;
// m.cppm
export module m;
import "foo.h";
use of a...
```
BTW, given the semantics of header units and clang modules are pretty similar,
we should use `isHeaderModule ` in most cases.
================
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
----------------
iains wrote:
> 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,
>
It looks good enough to me to check that `ND` lives in a module. And from what
I profiled, the lookup process is not hot really.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145965/new/
https://reviews.llvm.org/D145965
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
