Page MenuHomePhabricator

[clang][AST] Check context of record in structural equivalence.
ClosedPublic

Authored by balazske on Wed, Nov 3, 8:51 AM.

Details

Summary

The AST structural equivalence check did not differentiate between
a struct and a struct with same name in different namespace. When
type of a member is checked it is possible to encounter such a case
and wrongly decide that the types are similar. This problem is fixed
by check for the namespaces of a record declaration.

Diff Detail

Event Timeline

balazske created this revision.Wed, Nov 3, 8:51 AM
balazske requested review of this revision.Wed, Nov 3, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 3, 8:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I very much like the code.
L1364 is uncovered according to my coverage results.

clang/lib/AST/ASTStructuralEquivalence.cpp
1360–1364

Shouldn't you also check the name as well?

1379

So, the previous DC1->isTranslationUnit() guards the DC1->getParent() to never be NULL; same for DC2.
Pretty neat.

clang/unittests/AST/StructuralEquivalenceTest.cpp
982–988

The outer-most namespace could also be inline.
I'd love to see a test about that as well.

1004–1009

extern "C" might be nested within other namespaces.
Probably worth testing that as well.

1025–1032

Please, also test if the name of the structs is mismatched.

How does it handle this case:

struct A {int x;};

struct A f(void) {
    struct A{} a;

    return a;

and this case:

struct b{};

struct a {int x;}           // visible outside of f
f(struct b {int x; } b1) {  // not visibile out of f
  return (struct a){.x = 1};
}

In the last case the the RecordDecl's a and b can be matched (not internal statements in a function) and this is similar to the existing test cases. The struct a and struct b already fails to match because different name.

clang/lib/AST/ASTStructuralEquivalence.cpp
1360–1364

Here DC1 or DC2 is already a function or translation unit, the name of it is not relevant.

1379

Change to !DC1->getParent() (from DC1->isTranslationUnit())? (The line can be moved to here to increase readability, so the break statement is at end of the loop.)

balazske updated this revision to Diff 387181.Mon, Nov 15, 2:28 AM

Updated tests.

martong accepted this revision.Fri, Nov 19, 2:22 AM

Thanks @balazske for updating the tests, I think this addresses @shafik 's questions. LGTM! Ping @shafik

This revision is now accepted and ready to land.Fri, Nov 19, 2:22 AM
balazske updated this revision to Diff 389111.Tue, Nov 23, 1:04 AM

Applied lint reformattings.

martong accepted this revision.Tue, Nov 23, 8:08 AM

Still LGTM! Let's land it!

This revision was landed with ongoing or failed builds.Wed, Nov 24, 8:20 AM
This revision was automatically updated to reflect the committed changes.

Looks like this breaks tests on windows: http://45.33.8.238/win/49558/step_7.txt

Please take a look and revert for now if it takes a while to fix.

balazske reopened this revision.Thu, Nov 25, 2:45 AM

Windows test failures occurred because wrong AST matchers in the tests. Tests will be updated to use named match.

This revision is now accepted and ready to land.Thu, Nov 25, 2:45 AM
balazske updated this revision to Diff 389709.Thu, Nov 25, 2:55 AM

Using correct AST match in the tests to fix failures on Windows.