Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix bug in CtLineElementComparator #3519

Merged
merged 9 commits into from
Aug 18, 2020

Conversation

MartinWitt
Copy link
Collaborator

@MartinWitt MartinWitt commented Aug 3, 2020

Core Problem

As we saw in issue #3514 analyzing Palladio-SimuLizar produces jdk erros while sorting.
The reason for this is because the CtLineComparator does not follow the compareTo contract. This patch fixes the comparator.

Some Math Stuff

A comparator must follow the compareTo contract in java. The compareTo contract defines two formulas:

(1) antisymmetric sgn(x.compareTo(y)) == -sgn(y.compareTo(x))
(2) transitive (x.compareTo(y)>0 && y.compareTo(z)>0) => x.compareTo(z)>0

Assuming we have 3 points X,Y and Z and all points have invalid positions.

compareTo(X,Y) == -1 ==> X < Y
compareTo(Y,Z) == -1 ==> Y < Z
compareTo(X,Z) == -1 ==> X < Z
Looks good till here but now:
compareTo(Z,X) == -1 => Z<X
here we violate the transitive requirement. Because Z is now less than X.
If we do the same after the change:
compareTo(X,Y) == 0 ==> X=Y
compareTo(Y,Z) == 0 ==> Y=Z
compareTo(X,Z) == 0 ==> X=Z
compareTo(Z,X) == 0 ==> Z=X
The change still follows the antisymmetric contract, because sign(0)=0== -sign(0).

Consequences

Some testcases relay on the old sorting, because order was different. Some refactoring for the testcases is needed.
There is a risk, that some users have problems with new correct ordering.

@monperrus
Copy link
Collaborator

Thanks for the fix!

2 failing tests are on generated code, we just have to regenerate the code.

So only CtClassTest.getConstructor is really failing, which suggests that we won't break many things.

Overall, the fix is reasonable to me.

@MartinWitt
Copy link
Collaborator Author

@monperrus merge/review ping

Copy link
Collaborator

@monperrus monperrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot for the update, see comment.

@MartinWitt MartinWitt changed the title WIP: fix: comparator bug Fix: comparator bug Aug 18, 2020
@monperrus monperrus changed the title Fix: comparator bug fix: fix bug in CtLineElementComparator Aug 18, 2020
@monperrus monperrus merged commit 2714ff0 into INRIA:master Aug 18, 2020
@monperrus
Copy link
Collaborator

Thanks a lot Martin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants