[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/DeclContextInternals.h:128
+    DeclsTy &Vec = *getAsVector();
+    DeclsTy::iterator I = std::find(Vec.begin(), Vec.end(), D);
+    return I != Vec.end();
----------------
a_sidorin wrote:
> `auto I = llvm::find(Vec, D)`?
I'd go with `return llvm::is_contained(Vec, D);`


================
Comment at: include/clang/AST/DeclContextInternals.h:125
 
+  bool containsInVector(NamedDecl *D) {
+    assert(getAsVector());
----------------
`const NamedDecl *D`


================
Comment at: include/clang/AST/DeclContextInternals.h:126
+  bool containsInVector(NamedDecl *D) {
+    assert(getAsVector());
+    DeclsTy &Vec = *getAsVector();
----------------
Please add a string literal to the assert explaining what is being checked here.


================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:1169
+///   matches 'a'.
+extern const internal::VariadicDynCastAllOfMatcher<Decl, IndirectFieldDecl>
+    indirectFieldDecl;
----------------
Be sure to update Registry.cpp and regenerate the AST matcher documentation by running clang\docs\tools\dump_ast_matchers.py.

This change feels orthogonal to the rest of the patch; perhaps it should be split out into its own patch?


================
Comment at: lib/AST/ASTImporter.cpp:4991
         if (IsStructuralMatch(D, FoundTemplate)) {
-          if (!IsFriend) {
-            Importer.MapImported(D->getTemplatedDecl(),
-                                 FoundTemplate->getTemplatedDecl());
-            return Importer.MapImported(D, FoundTemplate);
+          ClassTemplateDecl* TemplateWithDef = getDefinition(FoundTemplate);
+          if (D->isThisDeclarationADefinition() && TemplateWithDef) {
----------------
a_sidorin wrote:
> Space after '*'?
Space before the asterisk. ;-)


================
Comment at: lib/AST/ASTImporter.cpp:2729
+
+    const bool IsFriend = D->isInIdentifierNamespace(Decl::IDNS_TagFriend);
+    if (LexicalDC != DC && IsFriend) {
----------------
Drop the top-level `const` qualifier.


================
Comment at: lib/AST/ASTImporter.cpp:2730
+    const bool IsFriend = D->isInIdentifierNamespace(Decl::IDNS_TagFriend);
+    if (LexicalDC != DC && IsFriend) {
+      DC->makeDeclVisibleInContext(D2);
----------------
Elide braces; I'd probably sink the `IsFriend` into here since it's not used elsewhere.


================
Comment at: lib/AST/ASTImporter.cpp:5064
+    if (!ToTemplated->getPreviousDecl()) {
+      auto *PrevTemplated =
+          FoundByLookup->getTemplatedDecl()->getMostRecentDecl();
----------------
Do not use `auto` here as the type is not spelled out in the initialization.


================
Comment at: lib/AST/ASTImporter.cpp:5073
+
+  if (LexicalDC != DC && IsFriend) {
+    DC->makeDeclVisibleInContext(D2);
----------------
Elide braces.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53655/new/

https://reviews.llvm.org/D53655