iains added a comment. 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. > 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. > 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). 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. 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
