xazax.hun added a comment.
I think having both kinds of tests might make sense.
Overall, this looks good to me. Some nits inline.
================
Comment at: unittests/AST/ASTImporterTest.cpp:143
+class Fixture : public ::testing::Test {
+
----------------
I do not like the name of this class. It is like calling a variable as
`variable`.
================
Comment at: unittests/AST/ASTImporterTest.cpp:148
+
+ //Buffers for the contexts, must live in test scope
+ std::string ToCode;
----------------
Missing period on the end of the comment and missing space at the beginning.
================
Comment at: unittests/AST/ASTImporterTest.cpp:163
+ // We may have several From context and related translation units.
+ std::list<TU> FromTUs;
+
----------------
I think it might worth to have a note why do we use a list and not a vector.
================
Comment at: unittests/AST/ASTImporterTest.cpp:170
+ ASTContext &ToCtx = ToAST->getASTContext();
+ vfs::OverlayFileSystem *OFS = static_cast<vfs::OverlayFileSystem *>(
+
ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
----------------
Repeated type, use auto. Same below.
Are we sure these cast's will not fail? Shouldn't we use dynamic casts and
asserts?
================
Comment at: unittests/AST/ASTImporterTest.cpp:184
+ // Must not call more than once within the same test.
+ std::tuple<Decl *, Decl *>
+ getImportedDecl(StringRef FromSrcCode, Language FromLang,
----------------
I wonder if pair or tuple is the better choice here. I have no strong
preference just wondering.
================
Comment at: unittests/AST/ASTImporterTest.cpp:187
+ StringRef ToSrcCode, Language ToLang,
+ const char *const Identifier = "declToImport") {
+ ArgVector FromArgs, ToArgs;
----------------
Maybe StringRef for the Identifier?
================
Comment at: unittests/AST/ASTImporterTest.cpp:190
+ FromArgs = getBasicRunOptionsForLanguage(FromLang);
+ ToArgs = getBasicRunOptionsForLanguage(ToLang);
+
----------------
Maybe declaring in the same line would be better.
================
Comment at: unittests/AST/ASTImporterTest.cpp:227
+ assert(
+ std::find_if(FromTUs.begin(), FromTUs.end(), [&FileName](const TU &e) {
+ return e.FileName == FileName;
----------------
I prefer to capture FileName by value. Param names should be upper case. Same
below.
================
Comment at: unittests/AST/ASTImporterTest.cpp:231
+
+ ArgVector Args= getBasicRunOptionsForLanguage(Lang);
+ FromTUs.emplace_back(SrcCode, FileName, Args);
----------------
Formatting is off here.
================
Comment at: unittests/AST/ASTImporterTest.cpp:253
+ auto It =
+ std::find_if(FromTUs.begin(), FromTUs.end(), [&From](const TU &e) {
+ return e.TUDecl == From->getTranslationUnitDecl();
----------------
Capture From as value.
================
Comment at: unittests/AST/ASTImporterTest.cpp:996
+TEST_F(Fixture, TUshouldNotContainTemplatedDeclOfFunctionTemplates) {
+
+ Decl *From, *To;
----------------
Redundant new lines, same in most of the tests' beginning.
================
Comment at: unittests/AST/ASTImporterTest.cpp:1006
+ for (auto Child : TU->decls()) {
+ if (FunctionDecl *FD = dyn_cast<FunctionDecl>(Child)) {
+ if (FD->getNameAsString() == "declToImport") {
----------------
Repeated type.
================
Comment at: unittests/AST/ASTImporterTest.cpp:1033
+ for (auto Child : TU->decls()) {
+ if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(Child)) {
+ if (RD->getNameAsString() == "declToImport") {
----------------
Repeated type.
================
Comment at: unittests/AST/ASTImporterTest.cpp:1062
+ for (auto Child : TU->decls()) {
+ if (TypeAliasDecl *AD = dyn_cast<TypeAliasDecl>(Child)) {
+ if (AD->getNameAsString() == "declToImport") {
----------------
Repeated type.
================
Comment at: unittests/AST/ASTImporterTest.cpp:1091
+
+ // Check that the ClassTemplateSpecializationDecl is NOT the child of the TU
+ auto Pattern =
----------------
Missing periods in comments.
================
Comment at: unittests/AST/ASTImporterTest.cpp:1156
+ for (auto Child : cast<DeclContext>(D)->decls()) {
+ if (FieldDecl *FD = dyn_cast<FieldDecl>(Child)) {
+ if (FD->getNameAsString() != FieldNamesInOrder[i++]) {
----------------
Repeated type.
================
Comment at: unittests/AST/ASTImporterTest.cpp:1198
+ for (auto Child : cast<DeclContext>(D)->decls()) {
+ if (FieldDecl *FD = dyn_cast<FieldDecl>(Child)) {
+ if (FD->getNameAsString() != FieldNamesInOrder[i++]) {
----------------
Repeated type.
Repository:
rC Clang
https://reviews.llvm.org/D43967
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
