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

[Iain Sandoe via Phabricator via cfe-commits]

iains marked 3 inline comments as done.
iains added a comment.

I will look at the rest of the comments once back in the office.

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

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

It is a case that we have supported; the user puts in a use of a decl but forgets to import the module exporting it (I agree it is not _exactly_ a "typo" in terms of names, but the diagnostics counts it in the same way)



================
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:
> 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.
OK. I agree that is ideal; it remains to be seen if it is feasible.


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

Yes, indeed, I agree - if we were starting from scratch, we would do all this in the lower-level 'isVisible' - but since that is used in different ways by the higher levels, it seemed quite tricky to change.

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

maybe that might work, I believe that when I looked before it was going to be quite complex, I will look again.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145965