[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext

[Aleksei Sidorin via Phabricator via cfe-commits]

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