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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
