Ericsson / clang

Cross Translation Unit analysis capability for Clang Static Analyzer. (Fork of official clang at http://llvm.org/git/clang)
http://clang.llvm.org/
Other
15 stars 10 forks source link

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

Open balazske opened 5 years ago

balazske commented 5 years ago

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 commented 5 years ago

This PR could possibly supersede #625, right?

balazske commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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)