[PATCH] D53708: [ASTImporter] Add importer specific lookup

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a subscriber: jingham.
a_sidorin added a comment.

Hi Gabor,

I think it is a good patch with a nice test set. There are some mine comments inline. I'd also like to have LLDB guys opinion on it.



================
Comment at: lib/AST/ASTImporter.cpp:7605
+
+ASTImporter::ASTImporter(ASTImporterLookupTable *LookupTable,
+                         ASTContext &ToContext, FileManager &ToFileManager,
----------------
It's better to make `LookupTable` an optional parameter of the previous ctor and remove this one.


================
Comment at: lib/AST/ASTImporter.cpp:7657
+  } else {
+    // FIXME Can we remove this kind of lookup?
+    // Or lldb really needs this C/C++ lookup?
----------------
@shafik @jingham  Could you please address this question?


================
Comment at: lib/AST/ASTImporterLookupTable.cpp:52
+
+} // namespace unnamed
+
----------------
`// anonymous namespace` or just `// namespace`.


================
Comment at: lib/AST/ASTImporterLookupTable.cpp:84
+  DeclContext *DC = ND->getDeclContext()->getPrimaryContext();
+  remove(DC, ND);
+  DeclContext *ReDC = DC->getRedeclContext()->getPrimaryContext();
----------------
Should we remove DC-related entry if it is empty after deletion?


================
Comment at: lib/AST/ASTImporterLookupTable.cpp:95
+    return {};
+  const auto &FoundNameMap = DCI->second;
+  auto NamesI = FoundNameMap.find(Name);
----------------
Could you please add newlines after `if`s?


================
Comment at: lib/AST/ASTImporterLookupTable.cpp:121
+    DeclContext* DC = Entry.first;
+    std::string Primary = DC->getPrimaryContext() ? " primary " : " ";
+    llvm::errs() << "== DC: " << cast<Decl>(DC) << Primary <<  "\n";
----------------
1. StringRef
2. Do we need a space before newline?


================
Comment at: lib/AST/ASTImporterLookupTable.cpp:122
+    std::string Primary = DC->getPrimaryContext() ? " primary " : " ";
+    llvm::errs() << "== DC: " << cast<Decl>(DC) << Primary <<  "\n";
+    dump(DC);
----------------
Two spaces before "\n".


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:258
+    TranslationUnitDecl *ToTU) {
+  if (LookupTable)
+    return;
----------------
```if (!LookupTable)
  LookupTable = llvm::make_unique<ASTImporterLookupTable>(*ToTU); ```
?


================
Comment at: unittests/AST/ASTImporterTest.cpp:346
+    assert(ToTU);
+    if (LookupTablePtr)
+      return;
----------------
`if (!LookupTablePtr) ...`


================
Comment at: unittests/AST/ASTImporterTest.cpp:3843
+TEST_P(ASTImporterLookupTableTest, EmptyCode) {
+  auto *ToTU = getToTuDecl( "", Lang_CXX);
+  ASTImporterLookupTable LT(*ToTU);
----------------
Redundant space after paren.


================
Comment at: unittests/AST/ASTImporterTest.cpp:3845
+  ASTImporterLookupTable LT(*ToTU);
+}
+
----------------
Could you please explain what does this test do?


================
Comment at: unittests/AST/ASTImporterTest.cpp:3889
+        if (ND->getDeclName() == Name)
+          Found = ND;
+    }
----------------
Do we need break/early return here?


================
Comment at: unittests/AST/ASTImporterTest.cpp:3930
+
+  auto FindInDeclListOfDC = [](DeclContext *DC, DeclarationName Name) {
+    Decl *Found = nullptr;
----------------
This lambda is the same as in previous func so it's better to extract it into a helper.


================
Comment at: unittests/AST/ASTImporterTest.cpp:4020
+  const RecordDecl *RD = getRecordDeclOfFriend(FriendD);
+  auto *Y = FirstDeclMatcher<CXXRecordDecl>().match(ToTU, cxxRecordDecl(hasName("Y")));
+
----------------
This line exceeds 80 chars.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53708/new/

https://reviews.llvm.org/D53708