[PATCH] D26753: ASTImporter: improve support for C++ templates

[Sean Callanan via cfe-commits]

spyffe requested changes to this revision.
spyffe added a comment.
This revision now requires changes to proceed.

This looks amazing.  I have a few minor quibbles and a testing concern, but overall this looks like a great step forward!  Thank you!



================
Comment at: lib/AST/ASTImporter.cpp:458
+  }
+  return true;
+}
----------------
Is this really an appropriate default result?  I would argue for `false` here so that an error would propagate, as is typical in ASTImporter.
Note that this does disagree with the original source's `true` but I think that was because we knew we didn't handle anything, whereas now the assumption is we handle everything.


================
Comment at: lib/AST/ASTImporter.cpp:496
+        return false;
+      if (DN1->isIdentifier())
+        return IsStructurallyEquivalent(DN1->getIdentifier(),
----------------
We should probably also check whether `DN1->isIdentifier() == DN2->isIdentifier()`.


================
Comment at: lib/AST/ASTImporter.cpp:520
+  }
   return true;
 }
----------------
As above, I'd argue for `false` here now that we're flipping to the assumption that this code is complete.


================
Comment at: lib/AST/ASTImporter.cpp:4911
+
+    if (D->getTypeAsWritten()) {
+      TypeSourceInfo *TInfo = Importer.Import(D->getTypeAsWritten());
----------------
Could you assign this to a variable here, to avoid the redundant call a line below?


================
Comment at: lib/AST/ASTImporter.cpp:4931
     LexicalDC->addDeclInternal(D2);
+
   }
----------------
This blank line is probably not needed.


================
Comment at: lib/AST/ASTImporter.cpp:7035
 
 NestedNameSpecifierLoc ASTImporter::Import(NestedNameSpecifierLoc FromNNS) {
+  // Copied from NestedNameSpecifier mostly.
----------------
Is this function properly covered by the test?  I would like to see some deeply-neded name specifiers in the test, with entries for all the cases here.
If I'm missing the part of the test that covers this, please let me know.


https://reviews.llvm.org/D26753