[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/include/clang/Sema/Sema.h:2356
+  /// Determine whether the module M is part of the current named module.
+  bool isPartOfCurrentNamedModule(const Module *M) const {
+    if (!M || M->isGlobalModule())
----------------
ChuanqiXu wrote:
> While I am not a native speaker, I feel `isSameModuleWithCurrentTU` may be a better name.
actually, as noted, both cases are needed. So I have made that clear by splitting the function into two.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:5821-5832
+  } else if (Decl->hasLinkage() &&
+             Decl->getFormalLinkage() == Linkage::ModuleLinkage) {
+    Diag(UseLoc, diag::err_module_private_use)
+        << (int)MIK << Decl << Modules[0]->getFullModuleName();
+    Diag(Decl->getBeginLoc(), diag::note_suggest_export)
+        << (int)MIK
+        << FixItHint::CreateInsertion(Decl->getBeginLoc(), "export");
----------------
ChuanqiXu wrote:
> I feel like this can be another change. I'm a little bit confused since I feel the patch did multiple things at the same time again..
sure, we can refactor; the current patch is a placeholder to allow work to continue on `p1815`.


================
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:
> 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.
OK, we can always revisit performance later,



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145965