a_sidorin added a subscriber: jingham.
a_sidorin added a comment.
Hi Gabor,
I think it is a good patch with a nice test set. There are some mine comments
inline. I'd also like to have LLDB guys opinion on it.
================
Comment at: lib/AST/ASTImporter.cpp:7605
+
+ASTImporter::ASTImporter(ASTImporterLookupTable *LookupTable,
+ ASTContext &ToContext, FileManager &ToFileManager,
----------------
It's better to make `LookupTable` an optional parameter of the previous ctor
and remove this one.
================
Comment at: lib/AST/ASTImporter.cpp:7657
+ } else {
+ // FIXME Can we remove this kind of lookup?
+ // Or lldb really needs this C/C++ lookup?
----------------
@shafik @jingham Could you please address this question?
================
Comment at: lib/AST/ASTImporterLookupTable.cpp:52
+
+} // namespace unnamed
+
----------------
`// anonymous namespace` or just `// namespace`.
================
Comment at: lib/AST/ASTImporterLookupTable.cpp:84
+ DeclContext *DC = ND->getDeclContext()->getPrimaryContext();
+ remove(DC, ND);
+ DeclContext *ReDC = DC->getRedeclContext()->getPrimaryContext();
----------------
Should we remove DC-related entry if it is empty after deletion?
================
Comment at: lib/AST/ASTImporterLookupTable.cpp:95
+ return {};
+ const auto &FoundNameMap = DCI->second;
+ auto NamesI = FoundNameMap.find(Name);
----------------
Could you please add newlines after `if`s?
================
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";
----------------
1. StringRef
2. Do we need a space before newline?
================
Comment at: lib/AST/ASTImporterLookupTable.cpp:122
+ std::string Primary = DC->getPrimaryContext() ? " primary " : " ";
+ llvm::errs() << "== DC: " << cast<Decl>(DC) << Primary << "\n";
+ dump(DC);
----------------
Two spaces before "\n".
================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:258
+ TranslationUnitDecl *ToTU) {
+ if (LookupTable)
+ return;
----------------
```if (!LookupTable)
LookupTable = llvm::make_unique<ASTImporterLookupTable>(*ToTU); ```
?
================
Comment at: unittests/AST/ASTImporterTest.cpp:346
+ assert(ToTU);
+ if (LookupTablePtr)
+ return;
----------------
`if (!LookupTablePtr) ...`
================
Comment at: unittests/AST/ASTImporterTest.cpp:3843
+TEST_P(ASTImporterLookupTableTest, EmptyCode) {
+ auto *ToTU = getToTuDecl( "", Lang_CXX);
+ ASTImporterLookupTable LT(*ToTU);
----------------
Redundant space after paren.
================
Comment at: unittests/AST/ASTImporterTest.cpp:3845
+ ASTImporterLookupTable LT(*ToTU);
+}
+
----------------
Could you please explain what does this test do?
================
Comment at: unittests/AST/ASTImporterTest.cpp:3889
+ if (ND->getDeclName() == Name)
+ Found = ND;
+ }
----------------
Do we need break/early return here?
================
Comment at: unittests/AST/ASTImporterTest.cpp:3930
+
+ auto FindInDeclListOfDC = [](DeclContext *DC, DeclarationName Name) {
+ Decl *Found = nullptr;
----------------
This lambda is the same as in previous func so it's better to extract it into a
helper.
================
Comment at: unittests/AST/ASTImporterTest.cpp:4020
+ const RecordDecl *RD = getRecordDeclOfFriend(FriendD);
+ auto *Y = FirstDeclMatcher<CXXRecordDecl>().match(ToTU,
cxxRecordDecl(hasName("Y")));
+
----------------
This line exceeds 80 chars.
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
