martong accepted this revision. martong added a comment. This revision is now accepted and ready to land.
This patch is really useful and LGTM! Just found some minor things. ================ Comment at: lib/AST/ASTImporter.cpp:7058 + const SrcMgr::ExpansionInfo &FromEx = FromSLoc.getExpansion(); + SourceLocation ToSpLoc = Import(FromEx.getSpellingLoc()); + SourceLocation ToExLocS = Import(FromEx.getExpansionLocStart()); ---------------- Let's say we import a `SourceLocation` with `ASTImporter::Import(SourceLocation FromLoc)`. That calls into `ASTImporter::Import(FileID FromID)` where we again import other source locations. Could the initial `FromLoc` be equal with any of these locations (`FromEx.getSpellingLoc()`, `FromEx.getExpansionLocStart()`) ? My understanding is that this is not possible because we cannot have recursive macros, but please confirm. ================ Comment at: unittests/AST/ASTImporterTest.cpp:1521 +static void CompareSourceLocs(SourceLocation Loc1, SourceLocation Loc2, + SourceManager &SM1, SourceManager &SM2) { ---------------- Perhaps it would be more concise and less error prone to use a `FullSourceLoc` which wraps one simple `SourceLocation` and a `SourceManager`. ================ Comment at: unittests/AST/ASTImporterTest.cpp:1557 + SourceManager &FromSM = FromD->getASTContext().getSourceManager(); + CompareSourceRanges(ToD->getSourceRange(), FromD->getSourceRange(), ToSM, + FromSM); ---------------- ``` CompareSourceRanges(FullSourceRange{ToD->getSourceRange(), ToSM}, .. ``` ? Repository: rC Clang https://reviews.llvm.org/D47698 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits