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

In D145965#4431846 <https://reviews.llvm.org/D145965#4431846>, @ChuanqiXu wrote:

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

Because when we do typo checking, we relax the visibility conditions so that we can see some decl that might be hidden or misspelled - then we can say

  "you need to import module XX before using YY", 

or

  "did you mean ZZ"

(I would be happy if we did not need to do this in this patch, but not sure how we can work around it).



================
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:
> 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`?
I will reword.


================
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;
+
----------------
ChuanqiXu wrote:
> Let's use `!DeclBase::isInAnotherModuleUnit()` instead now.
'isInAnotherModuleUnit' is not precise - for this work we need to have :

"is in another module unit of the same named module"
"is in another module unit of a different named module"

so I have made two functions to do these two jobs (although they are in Sema now, we can move them either to decl or module - as suggested later)



================
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() ==
----------------
ChuanqiXu wrote:
> nit: I prefer this to be a freestanding function in Module.h. This looks slightly not good within Sema.
what about moving all the module tu-related tests to decl.cpp/h (or maybe to module.cpp/h)?



================
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.
----------------
ChuanqiXu wrote:
> It looks better to me if we can insert codes here
I am not sure exactly what you mean by "insert codes"?


================
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:
> > > 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.
I am not sure what you mean here - I would like us to get this lookup stuff fixed for 17, so will be working on it when back in the office (traveling today)

There is a different behaviour between cases where the entry is from an named module (but not the current one) and a different TU of the same named module.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145965