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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D145965#4431888 <https://reviews.llvm.org/D145965#4431888>, @iains wrote:

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

It is OK to see the misspelled decl. But it looks weird to me to see the hidden decls. I think such regression should be called correction.



================
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;
+
----------------
iains wrote:
> 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)
> 
Sounds not bad to me.


================
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() ==
----------------
iains wrote:
> 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)?
> 
I prefer moving these to Module.h since its operand is Module instead of Decl.


================
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.
----------------
iains wrote:
> ChuanqiXu wrote:
> > It looks better to me if we can insert codes here
> I am not sure exactly what you mean by "insert codes"?
Oh, this is a little bit historical. I mean it looks better to do the main job of the patch (correcting the visibility of internal linkage entities) if possible.


================
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) {
----------------
iains wrote:
> 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.
I mean the we should put the similar things together as much as possible. It looks that the codes are trying to correcting the visibilities returned from `isVisible()`. But this sounds not good. Since it implies that the result from `isVisible()` is not valid.

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

Maybe we can tried to solve this by adding other `isVisible` interfaces.

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

Yeah, but we still have one month. And even if we didn't get it in time. (Given the size of the patch) It is still OK to back port this before the first week of September. So we can be more relaxed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145965