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