rsmith added a comment. Generally this looks good, but it needs an accompanying test.
================ Comment at: include/clang/AST/ASTContext.h:1129-1130 + /// Retrieve the declaration for the type_info class type. + RecordDecl *getTypeInfoClassDecl() const; + ---------------- Please indicate somewhere that this is the non-standard MSVC `::type_info` type, not the standard `std::type_info` type. ================ Comment at: lib/Sema/SemaDecl.cpp:1464 if (NewM == OldM) return false; ---------------- Somewhere up here we're calling `getOwningModule()` but should be calling `getOwningModuleForLinkage()`. (Please upload patches with full context as described at https://llvm.org/docs/Phabricator.html) ================ Comment at: lib/Sema/SemaDecl.cpp:1467-1470 + // FIXME: The Modules TS does not specify how to handle inplicit types + // For now we will simply ignore the implicit global types + if (Old->isImplicit()) + return false; ---------------- Rather than doing this, please change `ASTContext::buildImplicitRecord` to `setModuleOwnershipKind(Decl::ModuleOwnershipKind::Unowned)` on the created declaration. Repository: rC Clang https://reviews.llvm.org/D52973 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits