[PATCH] D47698: [ASTImporter] import macro source locations

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hi Rafael,

I think the patch is great. But, honestly, I have never dealt with SourceLocation machinery closely, so some things are a bit unclear to me.



================
Comment at: lib/AST/ASTImporter.cpp:7058
+    const SrcMgr::ExpansionInfo &FromEx = FromSLoc.getExpansion();
+    SourceLocation ToSpLoc = Import(FromEx.getSpellingLoc());
+    SourceLocation ToExLocS = Import(FromEx.getExpansionLocStart());
----------------
r.stahl wrote:
> martong wrote:
> > 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.
> Yes, that was my understanding as well. If some compiler error is a macro expansion it eventually stops at some point while walking this chain.
If we have a macro referring another macro in the same file, will we import their FileID twice? First, during `Import(getSpellingLoc())` and then in this caller?


================
Comment at: lib/AST/ASTImporter.cpp:7060
+    SourceLocation ToExLocS = Import(FromEx.getExpansionLocStart());
+    SourceLocation ToExLocE = Import(FromEx.getExpansionLocEnd());
+    unsigned TokenLen = FromSM.getFileIDSize(FromID);
----------------
`ToExLocE` seems to be unused in the true branch below. Could we move it into 'else' branch?


================
Comment at: unittests/AST/ASTImporterTest.cpp:1537
+      R"(
+      #define MFOO(arg) arg = arg + 1
+
----------------
Could you please add a test with nested macros? I.e.
```
#define FUNC_INT void declToImport
#define FUNC FUNC_INT
FUNC(int a);
```


Repository:
  rC Clang

https://reviews.llvm.org/D47698