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

[Iain Sandoe via Phabricator via cfe-commits]

iains marked 2 inline comments as done.
iains added inline comments.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:2082
+    Module *DeclModule = SemaRef.getOwningModule(D);
+    if (DeclModule && !DeclModule->isModuleMapModule() &&
+        !SemaRef.isModuleUnitOfCurrentTU(DeclModule) &&
----------------
ChuanqiXu wrote:
> 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.
ah yes, that special case, I should have remembered.
OK I will adjust this at the next iteration (I think you mean `isHeaderLikeModule()`)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145965/new/

https://reviews.llvm.org/D145965