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

[Gabor Marton via Phabricator via cfe-commits]

martong marked 8 inline comments as done.
martong added inline 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.
----------------
a_sidorin wrote:
> Naming conventions require method names to start with a small letter.
Okay, I changed it. Unfortunately `ASTImporter` does not really follow the conventions. Most of the functions start with a capital letter, and some with a small case.
E.g.:
```
    /// Retrieve the file manager that AST nodes are being imported from.
    FileManager &getFromFileManager() const { return FromFileManager; }

    /// Report a diagnostic in the "to" context.
    DiagnosticBuilder ToDiag(SourceLocation Loc, unsigned DiagID);

```


================
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"
----------------
a_sidorin wrote:
> It looks like the only change done to this file is including a header. Are there any related changes that should be added?
Thank you, this is a good catch, I removed it.


================
Comment at: unittests/AST/ASTImporterTest.cpp:3845
+  ASTImporterLookupTable LT(*ToTU);
+}
+
----------------
a_sidorin wrote:
> 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.
Ok, I removed it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53708