a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land.
Hi Gabor, I like this change! LGTM, just a few nits. ================ Comment at: clang/unittests/AST/ASTImporterGenericRedeclTest.cpp:20 + +// FIXME put these structs and the tests rely on them into their own separate +// test file! ---------------- Is this FIXME obsolete now? ================ Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:37 +// FunctionDecl: +auto *ExternF = "void f();"; +auto *StaticF = "static void f();"; ---------------- Can we use StringRef (or, at least `const auto *`)? ================ Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:49 + ::testing::WithParamInterface<std::tuple<ArgVector, const char *>>; +// Fixture to test the redecl chain of Decls with the same visibility. Gtest +// makes it possible to have either value-parameterized or type-parameterized ---------------- Double spaces in the end of sentences look a bit strange. ================ Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:139 + + auto *ToF0 = FirstDeclMatcher<DeclTy>().match(ToTu, getPattern()); + auto *FromF1 = FirstDeclMatcher<DeclTy>().match(FromTu, getPattern()); ---------------- Optional: F0/F1 (where F stands for "function", I guess) can be replaced with D0/D1 (for decl) since the code is generic. ================ Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:157 + TranslationUnitDecl *FromTu1 = getTuDecl(getCode1(), Lang_CXX, "input1.cc"); + auto *FromF0 = + FirstDeclMatcher<DeclTy>().match(FromTu0, getPattern()); ---------------- This should fit a single line. ================ Comment at: clang/unittests/AST/ASTImporterVisibilityTest.cpp:190 + +bool ExpectLink = true; +bool ExpectNotLink = false; ---------------- const? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61786/new/ https://reviews.llvm.org/D61786 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
