iains marked an inline comment as done. iains added inline comments.
================ 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; ---------------- 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. 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
