[PATCH] D145965: [C++20][Modules] Fix incorrect visibilities in implementation units.

[Chuanqi Xu via Phabricator via cfe-commits]

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