ChuanqiXu added inline comments.
================ Comment at: clang/lib/Sema/SemaLookup.cpp:3859 + // C++20 [basic.lookup.argdep] p4.3 .. are exported + Module *FM = D->getOwningModule(); + // .. are attached to a named module M, do not appear in the ---------------- nit: Although it should be true due to D is ExportDeclContext, it looks better to add an assertion at the first sight. ================ Comment at: clang/lib/Sema/SemaLookup.cpp:3862-3863 + // translation unit containing the point of the lookup.. + if (FM->isModulePurview() && + (ModuleScopes.empty() || FM != ModuleScopes.back().Module)) { + for (auto *E : AssociatedClasses) { ---------------- ================ Comment at: clang/lib/Sema/SemaLookup.cpp:3864-3878 + for (auto *E : AssociatedClasses) { + // and have the same innermost enclosing non-inline namespace + // scope as a declaration of an associated entity attached to M + if (!E->hasOwningModule() || E->getOwningModule() != FM) + continue; + // TODO: maybe this could be cached when generating the + // associated namespaces / entities. ---------------- nit: how do you think about the suggested style? (not required) ================ Comment at: clang/lib/Sema/SemaLookup.cpp:3867 + // scope as a declaration of an associated entity attached to M + if (!E->hasOwningModule() || E->getOwningModule() != FM) + continue; ---------------- The std says `module` instead of `module unit`. So I think we should consider partition unit here. So: - We need to add a test about partition in the revision. - We need to add `isInSameModule` methods in other revisions. I know we've talked this several times before... I'll take a look. ================ Comment at: clang/lib/Sema/SemaLookup.cpp:3871-3873 + DeclContext *Ctx = E->getDeclContext(); + while (!Ctx->isFileContext() || Ctx->isInlineNamespace()) + Ctx = Ctx->getParent(); ---------------- Maybe it is worth to implement `getNonInlineEnclosingNamespaceContext` and we could simplify the code here: ================ Comment at: clang/lib/Sema/SemaOverload.cpp:6407 + getLangOpts().CPlusPlusModules && + MF->getTopLevelModule() != getCurrentModule()->getTopLevelModule()) { + Candidate.Viable = false; ---------------- I introduced `isModuleUnitOfCurrentTU` to simplify the code a little bit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129174/new/ https://reviews.llvm.org/D129174 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits