a.sidorin added a comment. Hello Gabor,
Sorry for the delay with review: the patch is pretty big and it was hard to find time for reviewing it. I like the patch but it requires some cleanup like formatting changes. Could you please clang-format new code? Also, I'd like to avoid code duplication. I pointed some places where it happens. ================ Comment at: unittests/AST/ASTImporterTest.cpp:167 + + // We may have several From context and related translation units. In each + // AST, the buffers for the source are handled via references and are set ---------------- context_s_; two spaces before 'In'. ================ Comment at: unittests/AST/ASTImporterTest.cpp:178 + // already exists then the file will not be created again as a duplicate. + void createVirtualFile(StringRef FileName, const std::string &Code) { + assert(ToAST); ---------------- Could you refactor `testImport()` to use `createVirtualFile()` to avoid code duplication? ================ Comment at: unittests/AST/ASTImporterTest.cpp:194 + // of the identifier into the To context. + // Must not call more than once within the same test. + std::tuple<Decl *, Decl *> ---------------- Must not be called? ================ Comment at: unittests/AST/ASTImporterTest.cpp:197 + getImportedDecl(StringRef FromSrcCode, Language FromLang, StringRef ToSrcCode, + Language ToLang, StringRef Identifier = "declToImport") { + ArgVector FromArgs = getBasicRunOptionsForLanguage(FromLang), ---------------- This magic constant was factored out in my review. ================ Comment at: unittests/AST/ASTImporterTest.cpp:198 + Language ToLang, StringRef Identifier = "declToImport") { + ArgVector FromArgs = getBasicRunOptionsForLanguage(FromLang), + ToArgs = getBasicRunOptionsForLanguage(ToLang); ---------------- This means that testing under MSVC is omitted. It caused a lot of buildbot headache before, so I strongly recommend to add it. ================ Comment at: unittests/AST/ASTImporterTest.cpp:202 + FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs); + TU &FromTu = FromTUs.back(); + ---------------- FromTU? ================ Comment at: unittests/AST/ASTImporterTest.cpp:226 + + Decl *Imported = Importer.Import(*FoundDecls.begin()); + assert(Imported); ---------------- `FoundDecls.front()` ================ Comment at: unittests/AST/ASTImporterTest.cpp:978 + R"( +template<typename _T> +struct X {}; ---------------- Could you please format inline code? ================ Comment at: unittests/AST/ASTImporterTest.cpp:1090 + R"( + template<class T> + class Base {}; ---------------- Sometimes we start raw literal from the beginning of the line, sometimes - with indent. Is there any common style? ================ Comment at: unittests/AST/ASTImporterTest.cpp:1193 + + auto Check = [](Decl *D) -> bool { + std::array<const char*, 3> FieldNamesInOrder{{"a", "b", "c"}}; ---------------- Code duplication with upper example. In my patch, I introduced a matcher for this; I think it can be reused here. ================ Comment at: unittests/AST/ASTImporterTest.cpp:1254 + std::tie(From, To) = getImportedDecl( + R"( + void declToImport() {} ---------------- This can be written on single line. ================ Comment at: unittests/AST/ASTImporterTest.cpp:1365 + + // There must be only one imported FunctionDecl ... + EXPECT_TRUE(FirstDeclMatcher<FunctionDecl>().match(ToTU, Pattern) == ---------------- It looks like this piece of code duplicates below several times. If so, I think it should be factored into a separate function. ================ Comment at: unittests/AST/ASTImporterTest.cpp:1443 + ImportDefinitionThenPrototype) { + auto Pattern = functionDecl(hasName("f")); + ---------------- This `Pattern` is repeated often. ================ Comment at: unittests/AST/ASTImporterTest.cpp:1494 + EXPECT_TRUE(!ProtoD->doesThisDeclarationHaveABody()); + FunctionDecl* DefinitionD = LastDeclMatcher<FunctionDecl>().match(ToTU, Pattern); + EXPECT_TRUE(DefinitionD->doesThisDeclarationHaveABody()); ---------------- FunctionDecl *DefinitionD (same upper). ================ Comment at: unittests/AST/ASTImporterTest.cpp:1526 + +TEST_F(ImportFunctions, + OverriddenMethodsShouldBeImported) { ---------------- This fits 80-char limit, no need to split. ================ Comment at: unittests/AST/DeclMatcher.h:33 + template <typename MatcherType> + NodeType *match(const Decl *D, const MatcherType &AMatcher) { + MatchFinder Finder; ---------------- We can use `ASTContext &` directly instead of `const Decl *`. ================ Comment at: unittests/AST/DeclMatcher.h:48 +class DeclCounter : public MatchFinder::MatchCallback { + unsigned count = 0; + void run(const MatchFinder::MatchResult &Result) override { ---------------- Member names should start with capital: `Count`. ================ Comment at: unittests/AST/DeclMatcher.h:59 + MatchFinder Finder; + Finder.addMatcher(AMatcher.bind(""), this); + Finder.matchAST(D->getASTContext()); ---------------- This will cause confusing miscompile if `AMatcher` is not a `BindableMatcher` - like matchers defined with `AST_MATCHER`-like macros, for example. Isn't better to write this call like this: `Finder.addMatcher(BindableMatcher<NodeType>(AMatcher).bind(""), this);` 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