[PATCH] D52973: Add type_info predefined decl

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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