PHP 7 - Wrong Vector/Map/Set equality

[tyx]

I think creating a different issue from #65 is a better idea.

Current problem

var_dump(new \Ds\Set([1, 3]) == new \Ds\Set([1, 2, 3])); // true

it makes me very sad as I definitively can't rely on Ds in my tests. And I use Ds everywhere as it does a really nice job (thanks for that anyway)

Solution

Is there any reason why this behavior ? And is it a bug or a known behavior ? Looks too big that anyone complain about it ^^

Whatever I'm definitively open to contribute event if my C skill are very old because it cannot stay like that.

I already had a look on the (clean) code.
I guess we need to implements the compare_objects handlers on the internal htable ? Looks pretty obvious, I know. May be we can even use the toArray and rely on array::compare_objects ?
Is there any stuff that prevent us to do that ?

Anyway, let me know what I can do to help on this very annoying issue.

Thanks

[rtheunissen]

@tyx we would need to implement compare_objects on the zend objects, ie. php_ds_set etc. The internal htable would do the actual work though, eg.:

• compare_objects handler
• Check if the object is an instance of Set
• Access the internal set
• ds_set_equals(ds_set_t *a, ds_set_t* b)
• ds_htable_equals(ds_htable_t *a, ds_htable_t *b)

I'm guessing we'd iterate through both the table's keys and values and check if they're equal? (Or in the case of a Set, only the keys as all the values would be undefined).

toArray would be a performance hit, might also run into non-scalar keys.

Sorry about this annoying issue - keen to get it implemented and tested though.

There's also the obvious win if the object being compared to is the same instance. ⚡️

[tyx]

Looks nice ! Thanks you for your fast answer.

toArray would be a performance hit, might also run into non-scalar keys.

Indeed !

Sorry about this annoying issue - keen to get it implemented and tested though.

Compared to the benefits we get to use your extension, it's not a big deal in fact ;)

There's also the obvious win if the object being compared to is the same instance.

Yes ! This one works actually

[rtheunissen]

@nikic would this actually be implicit if we implement the get_properties handler?

[nikic]

@rtheunissen Yes-ish. I'd assume you wouldn't want to have the same comparison semantics as a property comparison would give you. In particular, those would be weak and I doubt you'd want to have Set(["foo"]) == Set([true]), right?

[rtheunissen]

The only issue here is that we can't implement this in the polyfill. 🙅‍♂️

[rtheunissen]

Only option would be to implement Hashable and use equals for equality checks. Would never need > and < anyway.

[shouze]

@rtheunissen ok so the main issue is equality so it's not a big issue about < and >. And yes why not with Hashable, we're ok that Set, Vector, ... don't implement it ATM? This is what you've planned to solve the issue.

[rtheunissen]

@shouze happy to do it with equals. Added to 2.0, see #65