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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
