a.sidorin added a comment. Hi Gabor,
I don't feel I'm a right person to review AST-related part so I'm adding other reviewers. What I'm worrying about is that there is no test to check if our changes in removeDecl are correct. Maybe https://reviews.llvm.org/D44100 is a right patch for this but we should land it first or set dependencies properly. Regarding ASTImporter, you can find my comments inline. ================ Comment at: lib/AST/DeclBase.cpp:1386 + // Do not try to remove the declaration if that is invisible to qualified + // lookup. E.g. template sepcializations are skipped. + if (shouldBeHidden(ND)) return; ---------------- specializations ================ Comment at: unittests/AST/ASTImporterTest.cpp:1770 +TEST(ImportExpr, ImportClassTemplatePartialSpecialization) { + MatchVerifier<Decl> Verifier; ---------------- These tests seem to be for ImportDecl, not for ImportExpr. ================ Comment at: unittests/AST/ASTImporterTest.cpp:1803 + + testImport(Code, Lang_CXX11, "", Lang_CXX11, Verifier, namespaceDecl()); +} ---------------- Check for namespaceDecl() looks too weak because import of NamespaceDecl succeeds even if import of its nested decls fails. Same below. ================ Comment at: unittests/AST/ASTImporterTest.cpp:1827 + +TEST_P(ASTImporterTestBase, DISABLED_ImportOfRecordWithDifferentFriends) { + Decl *ToR1; ---------------- For this change, we should create a separate patch. Repository: rC Clang https://reviews.llvm.org/D46835 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits