Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Make a thorough test of == and =:= for all basic types #18

Open
krestenkrab opened this Issue · 6 comments

2 participants

@krestenkrab
Owner

I am quite certain that we have not covered all the bases for equals operations (and perhaps comparisons too), and it would be good to have a more complete test base for == and =:=.

We need to evaluate what java.lang.Object.equals() should be bound to, ==, =:= or something else. I think it needs to be bound to =:= (so that e.g. 0 and 0.0 are different in equals/hash maps).

Perhaps, to make things perfectly clear, EObject should have erl_equals(other) and erl_equals_exact(other); and then have EObject.equals(java.lang.Object) call one of those.

But before we can commence, we need some more consistent test.

@eriksoe

I can make a comparison test case for this.
Don't know if that's what you had in mind - I guess we need to cover all kinds of list representations, for example? - but it will be a start (and is easy to make).

@eriksoe

I see you did that...
I've made some amendments: Firstly, there's no reason to compare both ways - the matrix test style takes care of that. Secondly, we need to cover e.g. all possible list representations. Ideally, we should check that equal_tests has complete code coverage of the comparison code...
I found an issue; it appears to be in EBigString.

@eriksoe

Fixed - but found another case: "ab" vs [97.0, 98.0].
Not sure if compare_same() is supposed to implement '==' or '=:=' ?

@krestenkrab
Owner

Compare same is supposed to be used for < > =< etc. Is (A >= B) and (A =< B) equivalent to == or =:= ? I would presume ==. Hence, compare_same() should implement ==-ish semantics.

Regarding the bug that "ab" =:= [97.0, 98.0] yields true right now, equalsExactly() for all lists needs to apply equalsExactly to it's pointwise elements.

@eriksoe

OK. Then the problem is that EObject.equalsExactly() calls compareTo() which uses compare_same(). And the only list classes which overrides equalsExactly() are EString and EBigString; EList inherits the EObject version.
I don't know if the EObject version makes much sense; could be it should be declared abstract instead. It certainly doesn't work for composite terms (and the atomic once all seem to override it).

(An observation: if we want compareTo() to behave like Erlang's '<', and equals() to behave like Erlang's '=:=', then compareTo() is not "consistent with equals"; this is allowed, but sorted collections then won't work for these objects.)

@eriksoe

At present we have the problem that

  • EObject.equals() has compares-same semantics
    • which makes hash-maps break whenever we happen to insert equivalent integer and float keys which happen to have the same hash value (granted, it appears to work OK for [1;1000000]),
  • ETS's ordered_set is implemented using TreeSets, which actually need the compare_same semantics.

The solution which I can see is to make equals() have exact-match semantics, and adapt the TreeSet used in ETS so that on insertion, all key objects are wrapped in an adapter which redirects equals() to compareSame(). (Yes, as described that'll be two adapters - one for TreeSets and one for their keys. The latter one is the essential one though.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.