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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
