[PATCH] D36589: Add support for remembering origins to ExternalASTMerger

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Thanks for the additional docs! More comments below.



================
Comment at: lib/AST/ExternalASTMerger.cpp:116
+    if (auto *ToDC = dyn_cast<DeclContext>(To)) {
+      logs() << "(ExternalASTMerger*)" << (void*)&Parent
+             << " imported (DeclContext*)" << (void*)ToDC
----------------
Can you conditionalize the logging?


================
Comment at: lib/AST/ExternalASTMerger.cpp:126
+          Parent.HasImporterForOrigin(*FromOrigins.at(FromDC).AST)) {
+        logs() << "(ExternalASTMerger*)" << (void*)&Parent
+               << " forced origin (DeclContext*)"
----------------
Same here.


================
Comment at: lib/AST/ExternalASTMerger.cpp:134
+      } else {
+        logs() << "(ExternalASTMerger*)" << (void*)&Parent
+               << " maybe recording origin (DeclContext*)" << (void*)FromDC
----------------
Same here.


================
Comment at: lib/AST/ExternalASTMerger.cpp:213
+    if (!DidCallback)
+      logs() << "(ExternalASTMerger*)" << (void*)this
+             << " asserting for (DeclContext*)" << (void*)DC
----------------
Same here.


================
Comment at: lib/AST/ExternalASTMerger.cpp:286
+  if (!FoundFromDC || !IsSameDC(FoundFromDC.get(), Origin.DC)) {
+    logs() << "(ExternalASTMerger*)" << (void*)this
+           << " decided to record origin (DeclContext*)" << (void*)Origin.DC
----------------
Ditto


================
Comment at: lib/AST/ExternalASTMerger.cpp:292
+  } else {
+    logs() << "(ExternalASTMerger*)" << (void*)this
+           << " decided NOT to record origin (DeclContext*)" << (void*)Origin.DC
----------------
Ditto


================
Comment at: tools/clang-import-test/clang-import-test.cpp:232
+
+struct CIAndOrigins {
+  using OriginMap = clang::ExternalASTMerger::OriginMap;
----------------
Can you also add a brief documentation for this one? 


================
Comment at: tools/clang-import-test/clang-import-test.cpp:242
+      return static_cast<ExternalASTMerger *>(Source)->GetOrigins();
+    else
+      return EmptyOriginMap;
----------------
No need for the `else` here.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:256
+  llvm::SmallVector<ExternalASTMerger::ImporterSource, 3> Sources;
+  for (CIAndOrigins &Import : Imports) {
+    Sources.push_back(
----------------
No need for curly braces here.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:326
         "Errors occured while parsing the expression.", std::error_code());
   } else {
     return std::move(CI);
----------------
No need for the `else` here.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:333
+  llvm::SmallVector<ExternalASTMerger::ImporterSource, 3> Sources;
+  for (CIAndOrigins &Import : Imports) {
+    Sources.push_back(
----------------
No need for curly braces here.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:354
       exit(-1);
     } else {
       ImportCIs.push_back(std::move(*ImportCI));
----------------
No need for the `else` here.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:365
   }
-  llvm::Expected<std::unique_ptr<CompilerInstance>> ExpressionCI =
-      Parse(Expression, Direct ? ImportCIs : IndirectCIs, DumpAST, DumpIR);
+  if (UseOrigins) {
+    for (auto &ImportCI : ImportCIs) {
----------------
No need for braces in this entire block


================
Comment at: tools/clang-import-test/clang-import-test.cpp:376
     exit(-1);
   } else {
+    Forget(*ExpressionCI, (Direct && !UseOrigins) ? ImportCIs : IndirectCIs);
----------------
No need for the `else` here.


https://reviews.llvm.org/D36589