a_sidorin added a comment. Hi Gabor, The patch looks almost good bu I have some comments inline.
================ Comment at: lib/AST/ASTImporter.cpp:3002 + // Check if we have found an existing definition. Returns with that + // definition if yes, otherwise returns null. ---------------- I like this lambda. To make the code even better, we can move this lambda outside of VisitFunctionDecl because this method is already pretty big. ================ Comment at: unittests/AST/ASTImporterTest.cpp:3862 + void CheckPreviousDecl(Decl *To0, Decl *To1) { + ASSERT_NE(To0, To1); ---------------- I don't like numbers. Maybe `To0` and `To1` are `LastDecl` and `ImportedDecl`, correspondingly? ================ Comment at: unittests/AST/ASTImporterTest.cpp:3881 + if (auto *From0F = dyn_cast<FunctionDecl>(To0)) { + auto *To0F = cast<FunctionDecl>(To0); + if (From0F->getTemplatedKind() == ---------------- To0 and From0F actually have the same value, and To0F is unused. ================ Comment at: unittests/AST/ASTImporterTest.cpp:3884 + FunctionDecl::TK_FunctionTemplateSpecialization) { + EXPECT_EQ(To0->getCanonicalDecl(), To1->getCanonicalDecl()); + // There may be a hidden fwd spec decl before a spec decl. ---------------- If I don't miss something, this assumption does not depend on To0 and To1 kind and can be moved out of the condition, to the function scope. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58668/new/ https://reviews.llvm.org/D58668 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits