[ASTImporter] Added visibility context check for CXXRecordDecl.

[balazske]

Summary

ASTImporter makes now difference between classes with same name in different
translation units if these are not visible outside. These classes are not linked
into one decl chain.

[shafik]

Minor comments, I am going to run check-lldb now.

unittests/AST/ASTImporterVisibilityTest.cpp

34 ↗

(On Diff #200962)

GetCXXRecordPattern feels more consistent.

49 ↗

(On Diff #200962)

const? It is not consistent w/ the previous declarations.

50 ↗

(On Diff #200962)

const? It is not consistent w/ the previous declarations.

[shafik]

LGTM

[balazske]

Thanks for reviewing.

Similar additions are planned to follow with function and variable template, enum, scoped enum, typedef, type alias.

unittests/AST/ASTImporterVisibilityTest.cpp

34 ↗

(On Diff #200962)

The exact decl name is not repeated in these variable names like "FunPattern" for functions. Using "class" instead of "CXXRecord" can be acceptable. I do not like ImportCXXRecordsVisibilityChain (GetRecPattern and ImportRecordsVisibilityChain may be good too, but we do not say "C++ record" for thing that is a class).

[balazske]

• Changed type of string constants to const.

cfe/trunk/lib/AST/ASTImporter.cpp

for (auto *FoundDecl : FoundDecls) {
         //    struct { struct A *next; } entry0;
         //    struct { struct A *next; } entry1;
         //  };
         //  struct X { struct { int a; }; struct { int b; }; }; // anon structs
         if (!SearchName)
           if (!IsStructuralMatch(D, FoundRecord, false))
             continue;
 
+        if (!hasSameVisibilityContext(FoundRecord, D))
+          continue;
+
         if (IsStructuralMatch(D, FoundRecord)) {
           RecordDecl *FoundDef = FoundRecord->getDefinition();
           if (D->isThisDeclarationADefinition() && FoundDef) {
             // FIXME: Structural equivalence check should check for same
             // user-defined methods.
             Importer.MapImported(D, FoundDef);
             if (const auto *DCXX = dyn_cast<CXXRecordDecl>(D)) {
               auto *FoundCXX = dyn_cast<CXXRecordDecl>(FoundDef);

cfe/trunk/unittests/AST/ASTImporterVisibilityTest.cpp

 struct GetFunPattern {
   using DeclTy = FunctionDecl;
   BindableMatcher<Decl> operator()() { return functionDecl(hasName("f")); }
 };
 struct GetVarPattern {
   using DeclTy = VarDecl;
   BindableMatcher<Decl> operator()() { return varDecl(hasName("v")); }
 };
+struct GetClassPattern {
+  using DeclTy = CXXRecordDecl;
+  BindableMatcher<Decl> operator()() { return cxxRecordDecl(hasName("X")); }
+};
 
 // Values for the value-parameterized test fixtures.
 // FunctionDecl:
 const auto *ExternF = "void f();";
 const auto *StaticF = "static void f();";
 const auto *AnonF = "namespace { void f(); }";
 // VarDecl:
 const auto *ExternV = "extern int v;";
 const auto *StaticV = "static int v;";
 const auto *AnonV = "namespace { extern int v; }";
+// CXXRecordDecl:
+const auto *ExternC = "class X;";
+const auto *AnonC = "namespace { class X; }";
 
 // First value in tuple: Compile options.
 // Second value in tuple: Source code to be used in the test.
 using ImportVisibilityChainParams =
     ::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
 // fixtures. However, we cannot have both value- and type-parameterized test
void TypedTest_ImportChain() {
     EXPECT_NE(ToD0, ToD1);
     EXPECT_EQ(ToD1->getPreviousDecl(), ToD0);
   }
 };
 
 // Manual instantiation of the fixture with each type.
 using ImportFunctionsVisibilityChain = ImportVisibilityChain<GetFunPattern>;
 using ImportVariablesVisibilityChain = ImportVisibilityChain<GetVarPattern>;
-// Value-parameterized test for the first type.
+using ImportClassesVisibilityChain = ImportVisibilityChain<GetClassPattern>;
+// Value-parameterized test for functions.
 TEST_P(ImportFunctionsVisibilityChain, ImportChain) {
   TypedTest_ImportChain();
 }
-// Value-parameterized test for the second type.
+// Value-parameterized test for variables.
 TEST_P(ImportVariablesVisibilityChain, ImportChain) {
   TypedTest_ImportChain();
 }
+// Value-parameterized test for classes.
+TEST_P(ImportClassesVisibilityChain, ImportChain) {
+  TypedTest_ImportChain();
+}
 
 // Automatic instantiation of the value-parameterized tests.
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionsVisibilityChain,
                         ::testing::Combine(
                            DefaultTestValuesForRunOptions,
                            ::testing::Values(ExternF, StaticF, AnonF)), );
 INSTANTIATE_TEST_CASE_P(
     ParameterizedTests, ImportVariablesVisibilityChain,
     ::testing::Combine(
         DefaultTestValuesForRunOptions,
         // There is no point to instantiate with StaticV, because in C++ we can
         // forward declare a variable only with the 'extern' keyword.
         // Consequently, each fwd declared variable has external linkage.  This
         // is different in the C language where any declaration without an
         // initializer is a tentative definition, subsequent definitions may be
         // provided but they must have the same linkage.  See also the test
         // ImportVariableChainInC which test for this special C Lang case.
         ::testing::Values(ExternV, AnonV)), );
+INSTANTIATE_TEST_CASE_P(
+    ParameterizedTests, ImportClassesVisibilityChain,
+    ::testing::Combine(
+        DefaultTestValuesForRunOptions,
+        ::testing::Values(ExternC, AnonC)), );
 
 // First value in tuple: Compile options.
 // Second value in tuple: Tuple with informations for the test.
 // Code for first import (or initial code), code to import, whether the `f`
 // functions are expected to be linked in a declaration chain.
 // One value of this tuple is combined with every value of compile options.
 // The test can have a single tuple as parameter only.
 using ImportVisibilityParams = ::testing::WithParamInterface<
void TypedTest_ImportAfterImport() {
     if (shouldBeLinked())
       EXPECT_EQ(ToD1->getPreviousDecl(), ToD0);
     else
       EXPECT_FALSE(ToD1->getPreviousDecl());
   }
 };
 using ImportFunctionsVisibility = ImportVisibility<GetFunPattern>;
 using ImportVariablesVisibility = ImportVisibility<GetVarPattern>;
+using ImportClassesVisibility = ImportVisibility<GetClassPattern>;
 
 // FunctionDecl.
 TEST_P(ImportFunctionsVisibility, ImportAfter) {
   TypedTest_ImportAfter();
 }
 TEST_P(ImportFunctionsVisibility, ImportAfterImport) {
   TypedTest_ImportAfterImport();
 }
 // VarDecl.
 TEST_P(ImportVariablesVisibility, ImportAfter) {
   TypedTest_ImportAfter();
 }
 TEST_P(ImportVariablesVisibility, ImportAfterImport) {
   TypedTest_ImportAfterImport();
 }
+// CXXRecordDecl.
+TEST_P(ImportClassesVisibility, ImportAfter) {
+  TypedTest_ImportAfter();
+}
+TEST_P(ImportClassesVisibility, ImportAfterImport) {
+  TypedTest_ImportAfterImport();
+}
 
 const bool ExpectLink = true;
 const bool ExpectNotLink = false;
 
 INSTANTIATE_TEST_CASE_P(
     ParameterizedTests, ImportFunctionsVisibility,
     ::testing::Combine(
         DefaultTestValuesForRunOptions,
::testing::Combine(
                           std::make_tuple(ExternV, StaticV, ExpectNotLink),
                           std::make_tuple(ExternV, AnonV, ExpectNotLink),
                           std::make_tuple(StaticV, ExternV, ExpectNotLink),
                           std::make_tuple(StaticV, StaticV, ExpectNotLink),
                           std::make_tuple(StaticV, AnonV, ExpectNotLink),
                           std::make_tuple(AnonV, ExternV, ExpectNotLink),
                           std::make_tuple(AnonV, StaticV, ExpectNotLink),
                           std::make_tuple(AnonV, AnonV, ExpectNotLink))), );
+INSTANTIATE_TEST_CASE_P(
+    ParameterizedTests, ImportClassesVisibility,
+    ::testing::Combine(
+        DefaultTestValuesForRunOptions,
+        ::testing::Values(std::make_tuple(ExternC, ExternC, ExpectLink),
+                          std::make_tuple(ExternC, AnonC, ExpectNotLink),
+                          std::make_tuple(AnonC, ExternC, ExpectNotLink),
+                          std::make_tuple(AnonC, AnonC, ExpectNotLink))), );
 
 } // end namespace ast_matchers
 } // end namespace clang