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