[PATCH] D47698: [ASTImporter] import macro source locations
Gabor Marton via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 19 08:05:00 PDT 2018
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
More information about the cfe-commits
mailing list