[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 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