ChuanqiXu added a comment. Did another quick look. And I feel the title and the summary of the page is not consistent with the patch itself. I think it is better to split this to make the change to focus on the entities with internal linkage. I don't know what's wrong with the module linkage things. Maybe we can file an issue ahead in next time.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11181 + "%select{declaration|definition|default argument|" + "explicit specialization|partial specialization}0 of %1 is private to module " + "'%2'">; ---------------- The 'private' here makes me to think about module private fragment while it is not true. I prefer to refactor it to something like "it is not exported". ================ 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()) ---------------- While I am not a native speaker, I feel `isSameModuleWithCurrentTU` may be a better name. ================ Comment at: clang/lib/Sema/SemaLookup.cpp:1782 assert(DeclModule && "hidden decl has no owning module"); + // If the owning module is visible, the decl is potentially acceptable. ---------------- It looks better to me if we can insert codes here ================ Comment at: clang/lib/Sema/SemaLookup.cpp:3912-3936 + if (Visible) { + if (!FM) + break; + assert (D->hasLinkage() && "an imported func with no linkage?"); + // Unless the module is a defining one for the + bool Ovr = true; + for (unsigned I = 0; I < CodeSynthesisContexts.size(); ++I) { ---------------- What's the intention for the change? And why is the current behavior bad without this? ================ Comment at: clang/lib/Sema/SemaLookup.cpp:4517 /// declarations are visible, false otherwise. static void checkCorrectionVisibility(Sema &SemaRef, TypoCorrection &TC) { TypoCorrection::decl_iterator DI = TC.begin(), DE = TC.end(); ---------------- What if we don't touch the typo part? I am still confusing. ================ 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"); ---------------- 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.. 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