iains marked 2 inline comments as done. iains added inline comments.
================ Comment at: clang/include/clang/Sema/Sema.h:2356 + /// Determine whether the module M is part of the current named module. + bool isPartOfCurrentNamedModule(const Module *M) const { + if (!M || M->isGlobalModule()) ---------------- ChuanqiXu wrote: > While I am not a native speaker, I feel `isSameModuleWithCurrentTU` may be a > better name. actually, as noted, both cases are needed. So I have made that clear by splitting the function into two. ================ Comment at: clang/lib/Sema/SemaLookup.cpp:5821-5832 + } else if (Decl->hasLinkage() && + Decl->getFormalLinkage() == Linkage::ModuleLinkage) { + Diag(UseLoc, diag::err_module_private_use) + << (int)MIK << Decl << Modules[0]->getFullModuleName(); + Diag(Decl->getBeginLoc(), diag::note_suggest_export) + << (int)MIK + << FixItHint::CreateInsertion(Decl->getBeginLoc(), "export"); ---------------- ChuanqiXu wrote: > I feel like this can be another change. I'm a little bit confused since I > feel the patch did multiple things at the same time again.. sure, we can refactor; the current patch is a placeholder to allow work to continue on `p1815`. ================ Comment at: clang/lib/Sema/SemaLookup.cpp:2101 + if (isVisible(SemaRef, ND)) { + if (SemaRef.getLangOpts().CPlusPlusModules && ND->isFromASTFile()) { + // The module that owns the decl is visible; However ---------------- ChuanqiXu wrote: > iains wrote: > > ChuanqiXu wrote: > > > Let's not use `isFromASTFile()`. It is a low level API without higher > > > level semantics. I think it is good enough to check the module of ND. > > lookup is very heavily used; the purpose of the isFromAST() check is to > > short-circuit the more expensive checks when we know that a decl must be in > > the same TU (i.e. it is not from an AST file). > > > > If we can find a suitable inexpensive check that has better semantics, I am > > fine to change this, > > > It looks good enough to me to check that `ND` lives in a module. And from > what I profiled, the lookup process is not hot really. OK, we can always revisit performance later, 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