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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Did another quick look. And I feel the title and the summary of the page is not consistent with the patch itself. I think it is better to split this to make the change to focus on the entities with internal linkage. I don't know what's wrong with the module linkage things. Maybe we can file an issue ahead in next time.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11181
+  "%select{declaration|definition|default argument|"
+  "explicit specialization|partial specialization}0 of %1 is private to module "
+  "'%2'">;
----------------
The 'private'  here makes me to think about module private fragment while it is not true. I prefer to refactor it to something like "it is not exported".


================
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())
----------------
While I am not a native speaker, I feel `isSameModuleWithCurrentTU` may be a better name.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:1782
   assert(DeclModule && "hidden decl has no owning module");
 
+  // If the owning module is visible, the decl is potentially acceptable.
----------------
It looks better to me if we can insert codes here


================
Comment at: clang/lib/Sema/SemaLookup.cpp:3912-3936
+          if (Visible) {
+            if (!FM)
+              break;
+            assert (D->hasLinkage() && "an imported func with no linkage?");
+            // Unless the module is a defining one for the
+            bool Ovr = true;
+            for (unsigned I = 0; I < CodeSynthesisContexts.size(); ++I) {
----------------
What's the intention for the change? And why is the current behavior bad without this?


================
Comment at: clang/lib/Sema/SemaLookup.cpp:4517
 /// declarations are visible, false otherwise.
 static void checkCorrectionVisibility(Sema &SemaRef, TypoCorrection &TC) {
   TypoCorrection::decl_iterator DI = TC.begin(), DE = TC.end();
----------------
What if we don't touch the typo part? I am still confusing.


================
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");
----------------
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..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145965