Added check for scope in structural equivalence (experimental). #629

balazske · 2019-03-08T11:32:54Z

I am not sure if this is the best solution but the tests do not fail. The CI tests should be tried. (This is not the final code.)

martong · 2019-03-08T13:30:53Z

lib/AST/ASTStructuralEquivalence.cpp

@@ -338,6 +430,23 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
  llvm_unreachable("Invalid template argument kind");
 }

+/// Determine whether two template argument lists are equivalent.


Is this hunk related?

Ok, I see you use it when comparing the contexts.

The function is used in IsEquivalentContext.

martong · 2019-03-08T13:31:13Z

lib/AST/ASTStructuralEquivalence.cpp

@@ -1633,6 +1742,8 @@ unsigned StructuralEquivalenceContext::getApplicableDiagnostic(
    return diag::warn_odr_parameter_pack_non_pack;
  case diag::err_odr_non_type_parameter_type_inconsistent:
    return diag::warn_odr_non_type_parameter_type_inconsistent;
+  default:


This is probably not related too.

I wanted to make a build warning go away (I have another one in ASTImporterTest.cpp, probably a new PR can be created to correct this).

martong · 2019-03-08T13:35:14Z

lib/AST/ASTStructuralEquivalence.cpp

+  if (auto *ND1 = dyn_cast<NamedDecl>(D1)) {
+    auto *ND2 = dyn_cast<NamedDecl>(D2);
+    if (!ND2)
+      return false;


What if here we would just get the two DeclContexts from D1 and D2 and we would compare them by either NamedDecl::getQualifiedNameAsString or by index::generateUSRForDecl?
The USR seems more precise and both seems to be slow.
About the slowness we could add yet another local cache to the StructuralEquivalenceContext class.

How is it different than what we had previously in #624 ?
This time we would compare only the containing scopes by string and not the Decls themselves.

What I am saying here is just an alternative solution to consider, not necessarily better ...

I want to compare only scope, not the decl name itself (that is done already). Probably this was the reason for some test failures when the qualified name was used.

martong · 2019-03-08T13:40:54Z

lib/AST/ASTStructuralEquivalence.cpp

+  }
+}
+
+static bool IsEquivalentContext(StructuralEquivalenceContext &Context, const ContextVector &Contexts1, const ContextVector &Contexts2) {


I am afraid that we are going to miss too much cases here by implementing this logic all from scratch. A similar logic is already implemented in either NamedDecl::getQualifiedNameAsString or by index::generateUSRForDec. I think we should reuse the USR version ... see my other comment too

The USR seems to be usable but again a string-like (slow?) check.

I am not sure, I think we should maybe try both solutions.

I like the context vector based solution you have and maybe that is the winner because seems much faster. I am just afraid that we are going to miss something or make errors when implementing that. We can't make big mistakes however when comparing strings.

martong · 2019-03-08T14:28:17Z

This PR could possibly supersede #625, right?

martong · 2019-03-08T14:33:28Z

lib/AST/ASTStructuralEquivalence.cpp

+
+using ContextVector = llvm::SmallVector<const DeclContext *, 4>;
+
+static void GetContexts(ContextVector &Contexts, const NamedDecl *ND) {


Does it handle transparent contexts correctly?

How should these be handled? They can be skipped here.

martong · 2019-03-08T14:46:22Z

lib/AST/ASTStructuralEquivalence.cpp

+    const DeclContext *DC2 = *DC2I;
+    ++DC2I;
+
+    if (const auto *Spec1 = dyn_cast<ClassTemplateSpecializationDecl>(DC1)) {


We should compare the template args with function template specializations (FunctionDecl with TK_MemberSpecialization TK_FunctionTemplateSpecialization) too.

martong · 2019-03-08T14:47:40Z

lib/AST/ASTStructuralEquivalence.cpp

+        OS << *ED;
+      else
+        continue;*/
+    } else if (const auto *ND1 = dyn_cast<NamedDecl>(DC1)) {


Perhaps we could start with this and then an early return (continue)?

martong · 2019-03-08T14:49:13Z

lib/AST/ASTStructuralEquivalence.cpp

+  }
+}
+
+static bool IsEquivalentContext(StructuralEquivalenceContext &Context, const ContextVector &Contexts1, const ContextVector &Contexts2) {


I'd split this function into two:

comparing all the contexts (basically the for loop)

comparing two contexts

martong · 2019-03-08T14:51:50Z

lib/AST/ASTStructuralEquivalence.cpp

+}
+
+static bool IsEquivalentContext(StructuralEquivalenceContext &Context, const ContextVector &Contexts1, const ContextVector &Contexts2) {
+  auto DC2I = Contexts2.begin();


Perhaps a check before to see whether the sizes are equal and then an early return if not would make it simpler.

martong · 2019-03-08T15:03:22Z

lib/AST/ASTStructuralEquivalence.cpp

+    Ctx = Ctx->getParent();
+
+  // Collect named contexts.
+  while (Ctx) {


This seems dangerous, what is the parent of a TranslationUnitDecl? Perhaps another guard is needed?
while (Ctx && !Ctx->isTranslationUnit())

balazske · 2019-03-08T16:19:19Z

Maybe the problems only occur when the scope is explicitly written in source code (like at X::A and Y::A only A is checked). So the "written string" could be compared, or only record and namespace decl (but not function). And the scope check can stop at the innermost encountered function.

martong · 2019-03-08T16:24:18Z

lib/AST/ASTStructuralEquivalence.cpp

+    if (auto *ID = MD->getClassInterface())
+      Ctx = ID;
+
+  while (Ctx && Ctx->isFunctionOrMethod())


I am not sure that we can skip functions. Think about this case:

namespace A { void foo() { struct B { int a; }; } void bar() { struct B { int a; }; } }

Here the first and last B have a different scope.

Strictly speaking, the equivalency check should handle these cases too, shouldn't it?

Yes theoretically it is possible to make such a structural equivalence check. But can this case occur from ASTImporter? During AST import of bar it is not possible to find (and compare) the B in foo? A DeclContext outside of foo should not have references to things inside foo, or can something "leak" to outside? (A special case can be the "struct definition in function parameter list" case.)

Ok, you convinced me, let's skip the functions.

martong · 2019-03-08T16:54:03Z

Maybe the problems only occur when the scope is explicitly written in source code (like at X::A and Y::A only A is checked). So the "written string" could be compared, or only record and namespace decl (but not function). And the scope check can stop at the innermost encountered function.

Not necessarily. Consider the below test case, where the same X denotes different types. So I think the problem is NOT limited to qualified names only, thus we have to do the scope check.

TEST_F(StructuralEquivalenceFunctionTest,
    Maci2) {
  auto t = makeNamedDecls(
      "namespace A { class X; } namespace C { using namespace A; void foo(X&); }",
      "namespace B { class X; } namespace C { using namespace B; void foo(X&); }",
      Lang_CXX11);
  EXPECT_FALSE(testStructuralMatch(t));
}

martong · 2019-03-08T16:57:54Z

There is another case which we should keep in mind:

TEST_F(StructuralEquivalenceFunctionTest,
    Maci) {
  auto t = makeNamedDecls(
      "class X; namespace A { using Y = X; } void foo(A::Y&);",
      "class X; namespace B { using Y = X; } void foo(B::Y&);",
      Lang_CXX11);
  EXPECT_TRUE(testStructuralMatch(t));
}

Here, even if the using decls have different scopes, the underlying type is the same. So if we just compare the scope of two using decls we might end up having the wrong decision. Probably we have to skip using decls from the scope check.

... And I think this case shows that if we just did a string based (USR or the other) comparison of the params then we would falsely report an inequivalence in this case.

... if we did the scope check with string on the underlying types then it would still work

... As a consequence we can't use the scope check in CheckCommonEquivalence, we have to branch based on the node kind and that means we have to put this to the CheckKindSpecificEquivalence.

Update: Just recognized, that the FunctionProtoType stores the canonical types for the arguments, so in this case it will be the X properly. (But I am not sure with other kind of types)

balazske added 2 commits March 7, 2019 07:11

Added failing test.

8cebd4d

Scope check at structural equivalence.

abc67c7

balazske force-pushed the struct_eq_scope branch from 48493c8 to abc67c7 Compare March 8, 2019 13:24

martong reviewed Mar 8, 2019

View reviewed changes

balazske mentioned this pull request Mar 8, 2019

namespace scoping is not handled properly in function and template parameters #627

Open

martong reviewed Mar 8, 2019

View reviewed changes

Another (improved?) version.

400fe8b

martong added the Experimental We still don't know whether this PR will be needed or not. label Apr 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added check for scope in structural equivalence (experimental). #629

Added check for scope in structural equivalence (experimental). #629

balazske commented Mar 8, 2019

martong Mar 8, 2019

martong Mar 8, 2019

balazske Mar 8, 2019

martong Mar 8, 2019

balazske Mar 8, 2019

martong Mar 8, 2019

martong Mar 8, 2019

balazske Mar 8, 2019

martong Mar 8, 2019

balazske Mar 8, 2019

martong Mar 8, 2019

martong commented Mar 8, 2019

martong Mar 8, 2019

balazske Mar 8, 2019

martong Mar 8, 2019

martong Mar 8, 2019

martong Mar 8, 2019

martong Mar 8, 2019

martong Mar 8, 2019

balazske commented Mar 8, 2019

martong Mar 8, 2019 •

edited

Loading

balazske Mar 11, 2019

martong Mar 11, 2019

martong commented Mar 8, 2019

martong commented Mar 8, 2019 •

edited

Loading


		using ContextVector = llvm::SmallVector<const DeclContext *, 4>;

		static void GetContexts(ContextVector &Contexts, const NamedDecl *ND) {

Added check for scope in structural equivalence (experimental). #629

Are you sure you want to change the base?

Added check for scope in structural equivalence (experimental). #629

Conversation

balazske commented Mar 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martong commented Mar 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balazske commented Mar 8, 2019

martong Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martong commented Mar 8, 2019

martong commented Mar 8, 2019 • edited Loading

martong Mar 8, 2019 •

edited

Loading

martong commented Mar 8, 2019 •

edited

Loading