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

[Gabor Marton via Phabricator via cfe-commits]

martong marked 18 inline comments as done.
martong added inline comments.


================
Comment at: lib/AST/ASTImporter.cpp:7605
+
+ASTImporter::ASTImporter(ASTImporterLookupTable *LookupTable,
+                         ASTContext &ToContext, FileManager &ToFileManager,
----------------
a_sidorin wrote:
> It's better to make `LookupTable` an optional parameter of the previous ctor and remove this one.
Okay, I have changed it to be a default initalized to nullptr parameter.


================
Comment at: lib/AST/ASTImporterLookupTable.cpp:84
+  DeclContext *DC = ND->getDeclContext()->getPrimaryContext();
+  remove(DC, ND);
+  DeclContext *ReDC = DC->getRedeclContext()->getPrimaryContext();
----------------
a_sidorin wrote:
> Should we remove DC-related entry if it is empty after deletion?
I don't think so.
The only place where we are going to use `remove` is in case we had an error during import (upcoming patch). Even if we had an error before, subsequent successfully imported Decls could be part of the same DC (so, chances are we would superfluously do a ping-pong of delete/create).


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


================
Comment at: unittests/AST/ASTImporterTest.cpp:3889
+        if (ND->getDeclName() == Name)
+          Found = ND;
+    }
----------------
a_sidorin wrote:
> Do we need break/early return here?
Yes, that's better to have.


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


Repository:
  rC Clang

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

https://reviews.llvm.org/D53708