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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> if we do not adjust the typo fixes, we will regress diagnostics.

What the kind of diagnostics will be regressed? I mean, it looks weird to me that we suggest typo fixes from hidden names.



================
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'">;
----------------
ChuanqiXu wrote:
> 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".
let's try rewording `private` to `invisible`?


================
Comment at: clang/include/clang/Sema/Sema.h:2373
+  // Determine whether the module M belongs to the  current TU.
+  bool isModuleUnitOfCurrentTU(const Module *M) const;
+
----------------
Let's use `!DeclBase::isInAnotherModuleUnit()` instead now.


================
Comment at: clang/include/clang/Sema/Sema.h:2375-2383
+  /// Determine whether the module MA is part of the same named module as MB.
+  bool arePartOfSameNamedModule(const Module *MA, const Module *MB) const {
+    if (!MA || MA->isGlobalModule())
+      return false;
+    if (!MB || MB->isGlobalModule())
+      return false;
+    return MA->getPrimaryModuleInterfaceName() ==
----------------
nit: I prefer this to be a freestanding function in Module.h. This looks slightly not good within Sema.


================
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) {
----------------
ChuanqiXu wrote:
> ChuanqiXu wrote:
> > ChuanqiXu wrote:
> > > What's the intention for the change? And why is the current behavior bad without this?
> > > What's the intention for the change? And why is the current behavior bad without this?
> > 
> > 
> Oh, I understand why I feel the code is not good since the decl with internal linkage or module linkage shouldn't be visible. So even if there are problems, we should handle them elsewhere.
Could we tried to address this? The change smells not so good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145965