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

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
Here are some new comments.



================
Comment at: lib/AST/ASTImporter.cpp:7658
+ASTImporter::FoundDeclsTy
+ASTImporter::FindDeclsInToCtx(DeclContext *DC, DeclarationName Name) {
+  // We search in the redecl context because of transparent contexts.
----------------
Naming conventions require method names to start with a small letter.


================
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";
----------------
a_sidorin wrote:
> 1. StringRef
> 2. Do we need a space before newline?
Point 2 is still not addressed. We print a string ending with a space and then print a newline. I think we can remove the trailing space from both string literals.


================
Comment at: tools/clang-import-test/clang-import-test.cpp:12
 #include "clang/AST/ASTImporter.h"
+#include "clang/AST/ASTImporterLookupTable.h"
 #include "clang/AST/DeclObjC.h"
----------------
It looks like the only change done to this file is including a header. Are there any related changes that should be added?


================
Comment at: unittests/AST/ASTImporterTest.cpp:3933
+  }
+  return Found;
+};
----------------
return nullptr, Found is actually unused.


================
Comment at: unittests/AST/ASTImporterTest.cpp:3845
+  ASTImporterLookupTable LT(*ToTU);
+}
+
----------------
martong wrote:
> a_sidorin wrote:
> > Could you please explain what does this test do?
> Well, it makes sure that we can build a lookup table for an empty file without any *assertion*. We may have an assertion in the ctor during traversing the AST. I consider this the most basic use case as it tests only the constructor.
> However, if you find it weird I can remove it.
Yes, I think it's better to remove it or check some invariants of an empty lookup table.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53708