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

Tue Mar 14 02:12:34 PDT 2023

ChuanqiXu added a comment.

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

> In D145965#4191973 <https://reviews.llvm.org/D145965#4191973>, @ChuanqiXu wrote:
>
>> Yeah, it is indeed a problem that `Sema::isModuleUnitOfCurrentTU` doesn't work for implementation units. And I have been thinking about this for a while. My thought is that the root cause may be that we shouldn't push the module interface to `Sema::ModuleScopes()` for the implementation unit. (We need some other small refactoring).
>
> I think it is not small refactoring - we make extensive use of "getCurrentModule()" to determine if we are processing a module and the interface to know which module the implementation belongs to.
>
> If we add the implicitly imported interface to "imports" instead of the module scope we will need to make sure that we deal with all these cases where we rely on the interface module for this information.
>
> I was thinking at one stage to add an 'Implementation' module Kind, but at the moment I do not think it is worth extending the size of the ModuleKind enum bit field for this (since we now have used up all the entries with the new 'ExplicitGlobalModuleFragment' case).  That also has a large impact also for relatively few uses.

Yeah, understood. I had a mental model but I am not sure if it can work very well as I expected. I'll try to give it a try recently.

>> I feel this is more natural and consistent.
>
> Yes, if we were starting from 0, that is so - but we have an implementation with long history - we would need to see the impact. [I am not against making this cleaner, but concerned about the amount of change].
>
>> I thought to take this one but we didn't use implementation units in the downstream and there is no related issue reports.
>
> I was wondering whether some of the reports filed naming duplicate and clashing symbols could be related, but I did not analyse.
>
>> So I didn't start to work on it... If you are not hurry, I'd like to take this. Otherwise I'd suggest you to try the above method I mentioned.
>
> I was working on this because it was affecting my P1815 <https://reviews.llvm.org/P1815> stuff - but I can use this patch as a temporary solution.

Thanks!

>> BTW, I suggest we file an issue first if we want to do something. So that we can avoid solving the same problem.
>
> There are (I think your)  FIXMEs in the testcases (but I do not know if there is a corresponding PR).

Oh sorry. My bad clearly. Once upon time, someone told me it is a bad idea to add FIXME in the test case... Now I understand.

>
>
>
> The checks for internal-linkage symbols and the improvements to diagnostics do not depend on **how** `Sema::isModuleUnitOfCurrentTU` is implemented, so that I think we could fix these problems and then deal with the refactoring later.
>
> In either case, I do not have much time to work more on this right now.

I'll try to make it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145965