ChuanqiXu added a comment. LGTM. Thanks.
================ Comment at: clang/include/clang/Sema/Sema.h:2278 + + /// For an interface unit, this is the implicitly imported interface unit. + clang::Module *ThePrimaryInterface = nullptr; ---------------- ================ Comment at: clang/lib/Sema/SemaDecl.cpp:1672-1677 + // A module implementation unit has visibility of the decls in its implicitly + // imported interface. + if (getLangOpts().CPlusPlusModules && NewM && OldM && + NewM->Kind == Module::ModuleImplementationUnit && + OldM->Kind == Module::ModuleInterfaceUnit && NewM->Name == OldM->Name) + return false; ---------------- iains wrote: > ChuanqiXu wrote: > > iains wrote: > > > ChuanqiXu wrote: > > > > I feel slightly better to add a field in Sema: > > > > > > > > ``` > > > > Module *PrimaryModuleInterface = nullptr; // Only valid if we're > > > > parsing a module unit. > > > > ``` > > > > > > > > Then we can avoid the string compare here. Also we can add it to > > > > `Sema::isUsableModule`. Now `Sema::isUsableModule` works because it > > > > falls to the string comparison. > > > I also thought about doing this, but decided that we should try to keep > > > the interface regular - so that all modules are treated the same. > > > > > > If we find that the string comparison for primary module name is a "hot > > > spot" then we should address it a different way - by caching an > > > identifier or similar (since we need to compare it in other places too). > > It just smells bad. I did profiling for the performance for modules these > > days. And it looks like the string comparison can hard to be the hot spot. > > The bottleneck lives in other places. The string comparison may be in the > > long tail. So we might not be able to prove this one by giving numbers. But > > it indeed smells bad and we can solve it simply. So let's make it. I think > > it is important to improve our code quality. > I have done this (for now) to make progress. > > but, actually, in this case, I do not agree with the mechanism > > .. instead of making this into a special case we should (as part of the > following fixes to lookup) make efficient helpers in module / sema that allow > us to see "same TU", "same named module" etc. That way we can improve the > performance of any one of those if it becomes an issue. > Yeah, I tried to use hash algorithm to avoid the string comparison. But there is a concern that the hash algorithm is not safe. And I found the string comparison is not the bottle neck then. So I think it may not be too late to perform it actually if there is related bug report. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126959/new/ https://reviews.llvm.org/D126959 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
