ChuanqiXu added a comment. In D126694#3561257 <https://reviews.llvm.org/D126694#3561257>, @iains wrote:
> so standard 10.4 ex 2 gives the expected output with this patch stack Cool, and I think we should add that one as a test if it works. Tests are always good. --- BTW, I guess we could remove dependencies with D126959 <https://reviews.llvm.org/D126959> , D126189 <https://reviews.llvm.org/D126189>, D126691 <https://reviews.llvm.org/D126691>. If I understand things correctly, there shouldn't be dependencies between them. ================ Comment at: clang/include/clang/AST/DeclBase.h:647 + bool isDiscardedInGlobalModuleFragment() const { + return getModuleOwnershipKind() == ModuleOwnershipKind::ModuleUnreachable; + } ---------------- Maybe we need to check if the owning module is global module or not. ================ Comment at: clang/include/clang/Sema/Sema.h:2273 + void HandleGMFReachability(Decl *D) { + if (D->isModuleUnreachable() && isCurrentModulePurview()) { + D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ReachableWhenImported); ---------------- I feel better if we would check if D lives in GMF. (We need to insert a check in isDiscardedInGlobalModuleFragment) ================ Comment at: clang/lib/AST/TextNodeDumper.cpp:1622 + if (D->isModuleUnreachable()) + OS << " ModuleUnreachable"; } ---------------- It may be better to keep the consistent style. ================ Comment at: clang/lib/AST/TextNodeDumper.cpp:1636 + if (D->isModuleUnreachable()) + OS << " ModuleUnreachable"; if (D->isFixed()) ---------------- ditto ================ Comment at: clang/lib/AST/TextNodeDumper.cpp:1647 + if (D->isModuleUnreachable()) + OS << " ModuleUnreachable"; if (D->isCompleteDefinition()) ---------------- ditto ================ Comment at: clang/lib/AST/TextNodeDumper.cpp:1679 + if (D->isModuleUnreachable()) + OS << " ModuleUnreachable"; ---------------- ditto ================ Comment at: clang/lib/AST/TextNodeDumper.cpp:1756 + if (D->isModuleUnreachable()) + OS << " ModuleUnreachable"; } ---------------- ditto ================ Comment at: clang/lib/AST/TextNodeDumper.cpp:1778 + if (D->isModuleUnreachable()) + OS << " ModuleUnreachable"; if (D->isNRVOVariable()) ---------------- ditto ================ Comment at: clang/lib/Sema/SemaModule.cpp:265 + Module *Mod; // The module we are creating. + Module *Interface = nullptr; // The interface fir an implementation. switch (MDK) { ---------------- Given `Interface` is only assigned in the case of `ModuleDeclKind::Implementation`, it looks possible to sink the declaration to the place it get assigned. In this way, we could avoid the confusion here. ================ Comment at: clang/lib/Sema/SemaModule.cpp:355-369 + if (Interface) { + // Make the import decl for the interface. + ImportDecl *Import = ImportDecl::Create(Context, CurContext, ModuleLoc, + Interface, Path[0].second); + VisibleModules.setVisible(Interface, ModuleLoc); + if (auto *InterfaceGMF = Interface->getGlobalModuleFragment()) + // The fact that the GMF is a seaprate sub-module is an implementation ---------------- So that we could hoist the codes. ================ Comment at: clang/lib/Sema/SemaModule.cpp:366 - // imported module decl. - return Import ? ConvertDeclToDeclGroup(Import) : nullptr; } ---------------- This comes from https://reviews.llvm.org/D126959. I feel like odd to return the import declaration that time. I feel like it is better to return the module declaration itself instead of the imported one. Let's return nullptr now and fix it in the future. ================ Comment at: clang/lib/Sema/SemaModule.cpp:964-965 + if (DeclRefExpr *DR = dyn_cast<DeclRefExpr>(S)) { + auto *D = DR->getFoundDecl(); +// llvm::dbgs() << "DR:"; D->dump(); + if (D->isModuleUnreachable()) { ---------------- ================ Comment at: clang/lib/Sema/SemaModule.cpp:978 +void Sema::markReachableGMFDecls(Decl *Orig) { + + if (isa<FunctionDecl>(*Orig)) { ---------------- It looks better to add assumption as an assertion . ================ Comment at: clang/lib/Sema/SemaModule.cpp:979-980 + + if (isa<FunctionDecl>(*Orig)) { + auto *FD = cast<FunctionDecl>(Orig); + for (auto *P : FD->parameters()) { ---------------- ================ Comment at: clang/lib/Sema/SemaModule.cpp:996-999 +// if (Changed) { +// llvm::dbgs() << "P:"; P->dump(); +// } + } ---------------- ================ Comment at: clang/lib/Sema/SemaModule.cpp:1003-1004 + FindDeclRefExprs(S); + } + } else if (isa<VarDecl>(*Orig)) { + auto *VD = cast<VarDecl>(Orig); ---------------- clang prefer shorter indentation. ================ Comment at: clang/lib/Sema/SemaModule.cpp:1009 + : VD->getASTContext().getUnqualifiedObjCPointerType(VD->getType()); + T.dump(); + } ---------------- Remove dump ================ Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:635-636 - if (ModulePrivate) { - // Module-private declarations are never visible, so there is no work to - // do. + if (ModulePrivate || + ModuleOwnership == Decl::ModuleOwnershipKind::ModuleUnreachable) { + // Module-private and unreachable declarations are never visible, so ---------------- Maybe we could make a new bool variable like `ModulePrivate` to keep the style consistent. So that we could write: ================ Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:649-652 + } else if (ModuleOwnership == Decl::ModuleOwnershipKind::ModuleUnreachable) { + D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModuleUnreachable); } else if (ModulePrivate) { D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate); ---------------- ================ Comment at: clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp:32 void test_early() { - in_header = 1; // expected-error {{missing '#include "foo.h"'; 'in_header' must be declared before it is used}} - // expected-note@*{{not visible}} + in_header = 1; // expected-error {{use of undeclared identifier 'in_header'}} ---------------- iains wrote: > ChuanqiXu wrote: > > This error message looks worse. I image I could hear users complaining > > this. (I doesn't say it is your fault since this is specified in standard > > and the standard cases are harder to understand). I want to know if this is > > consistent with GCC? > > This error message looks worse. I image I could hear users complaining > > this. (I doesn't say it is your fault since this is specified in standard > > and the standard cases are harder to understand). I want to know if this is > > consistent with GCC? > > As you say, the standard says "neither reachable nor visible" > if it's not reachable then. we would not have the information that it was > from header foo.h so we cannot form the nicer diagnostic. > > Perhaps we need to invent "reachable for diagnostics" ... which I'd rather > not divert effort to right now. > Maybe we could make a new diagnostic message. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126694/new/ https://reviews.llvm.org/D126694 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
