Page MenuHomePhabricator

[AST] Structural equivalence of methods

Authored by balazske on Jun 27 2018, 3:27 AM.



Added structural equivalence check for C++ methods.
Improved structural equivalence tests.
Added related ASTImporter tests.

Diff Detail


Event Timeline

balazske created this revision.Jun 27 2018, 3:27 AM
a.sidorin requested changes to this revision.Jun 27 2018, 5:42 AM

Hi Balázs,

I think that the test changes unrelated to C++ method equivalence should be moved into a separate patch.

851 ↗(On Diff #153034)
bool QualifiersEqual = Method1->isStatic() == Method2->isStatic() &&
                       Method1->isConst() == Method2->isConst() &&...

if (!QualifiersEqual)
  return false;
873 ↗(On Diff #153034)
if (Method1->getStmtKind() != Method2->getStmtKind())
  return false;

So we need to check only one declaration here and below.

892 ↗(On Diff #153034)

These two lines look a bit strange to me. For example, should we return false if one of the methods is an overloaded operator and other one is not? I guess these conditions need to be swapped or written like this:

if (Method1->getOverloadedOperator() != Method2->getOverloadedOperator())
  return false;

const IdentifierInfo *Literal1 = Method1->getLiteralIdentifier();
const IdentifierInfo *Literal2 = Method2->getLiteralIdentifier();
if (!::IsStructurallyEquivalent(Literal1, Literal2))
  return false;

omitting the first condition.

1558 ↗(On Diff #153034)

This test doesn't look related. Could you please move such tests into a separate patch?They make the review harder.

This revision now requires changes to proceed.Jun 27 2018, 5:42 AM
balazske added inline comments.Jun 27 2018, 11:58 PM
873 ↗(On Diff #153034)

What do you mean by getStmtKind? Probably getDeclKind? Anyway after the conversions it is good to check the pointers anyway, so there would be no big simplification.

892 ↗(On Diff #153034)

The single if with getOverloadedOperator should be enough. getLiteralIdentifier is not applicable for methods as far as I know (even if yes it is not related to the overloaded operator) (probably it is missing from check at FunctionDecl).

balazske updated this revision to Diff 154073.Jul 4 2018, 2:24 AM
  • Updates according to review comments
balazske marked 4 inline comments as done.Jul 4 2018, 2:28 AM
balazske added inline comments.Jul 4 2018, 3:25 AM
2454 ↗(On Diff #154073)

This change with Definition is needed to make the test ImportOfEquivalentMethod work. But this is a general problem with importing, I have a new test that fails and is not related to CXXMethodDecl. Add this new (not directly related) test or remove this change and disable the failing test?

a_sidorin accepted this revision.Jul 4 2018, 10:44 PM
a_sidorin added a subscriber: a_sidorin.

LG with a nit. Thank you!

489 ↗(On Diff #154073)

Could you add a comment why this test is disabled?

balazske updated this revision to Diff 154175.Jul 5 2018, 12:07 AM
  • Added comment into test StructuralEquivalenceRecordTest.DISABLED_Methods.
balazske marked an inline comment as done.Jul 5 2018, 12:15 AM
balazske added inline comments.
489 ↗(On Diff #154073)

Methods are not checked, there was no decision about to include this check or not. The problem was related to performance overhead and if order-independent check of methods is needed. (ASTImporter should keep order of imported fields and methods.) (This test is about equivalence of foo.)

a_sidorin added inline comments.Jul 8 2018, 3:32 PM
489 ↗(On Diff #154073)

You mean that imported decl have other order of methods? Do you mean implicit methods (because I see only a single method here)? If so, could you please note this in the comment?

balazske marked an inline comment as done.Jul 8 2018, 11:16 PM
balazske added inline comments.
489 ↗(On Diff #154073)

The problem with the ordering is in general at structural equivalence check. If a complex structure is imported with ASTImporter the ordering of fields and methods may change in the imported class (relative to the to-be imported) after the import. Either order-independent check is needed in structural equivalence check to match two classes with same methods but in different order, or import should not change ordering. Currently none of these is implemented, methods are not checked at all. The foos in this test should be non-equivalent because one different method x.

martong accepted this revision.Jul 10 2018, 3:15 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 11 2018, 2:42 AM
This revision was automatically updated to reflect the committed changes.