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

Already on GitHub? Sign in to your account

-hash is broken on CPArray #10

Closed
cappuccino opened this Issue May 3, 2009 · 22 comments

Comments

Projects
None yet
9 participants
@ghost

ghost commented May 3, 2009

It breaks the contract that two arrays which are -isEqual: also need to report the same -hash value.

Patch and tests included

It’s also the same way that GnuStep and Cocotron implement this - I was quite surprised how simple they implemented it.

:)

original LH ticket

This ticket has 1 attachment(s).

@ghost

ghost commented May 3, 2009

-hash is broken on CPArray

My point was that -hash works differently in Cappuccino than in Cocoa (something I previously had not realized).

We need access to __address as a method, which is what hash has been doing for us. To make -hash actually correct, we’ll need to add -address and update all of the cappuccino code that relies on hashes being globally unique.

by Ross Boucher

@ghost

ghost commented May 3, 2009

-hash is broken on CPArray

Too late to finish sentences...

The one which I provided does fit the contract and also behaves the same way that the others do.

by Martin Häcker

@ghost

ghost commented May 3, 2009

-hash is broken on CPArray

Yes, but it breaks parts of Cappuccino that expect the current behavior. For example, KeyValueObserving and CPWindow both rely on the value of -hash as a unique index into various data structures.

by Ross Boucher

@ghost

ghost commented May 3, 2009

-hash is broken on CPArray

Oh damn.

I just had a look around in the source and lots of objects like CPDictionary, CPData, CPDate and CPNull don’t overide -hash but should. (And most should probably also overide isEqual:)

Uh oh....

Yes, this is going to take some work.

by Martin Häcker

@ghost

ghost commented May 3, 2009

-hash is broken on CPArray

Can’t resist...

As a good person once said: In God we trust - for everything else we have UnitTests.

:)

by Martin Häcker

@ghost

ghost commented May 3, 2009

-hash is broken on CPArray

Why do both array’s need to return the same hash value? That doesn’t seem to be documented anywhere in the Cocoa docs.

This code seems incredibly dangerous to me. You’re essentially saying that all kinds of arrays that have nothing to do with each other could be considered equal. This will break anything that uses hash as a unique identifier (for example, KVO/Bindings, and I’m sure other code in Cappuccino).

by Ross Boucher

@ghost

ghost commented May 3, 2009

-hash is broken on CPArray

I’m not sure I understand you correctly, -[CPObject hash] does return __adress, I haven’t checked all the other objects, but all objects who redefine -isEqual: need their own implementation that matches the contract.

The one which I provided for CPArray - and I checked it against the two other Cocoa implementations I know, which do it the same way. (Also it makes sense: The Implementation needs to be FAST, and it needs to be the same when two objects are -isEqual:

I also checked against some examples on Cocoa and it behaves the same way.

by Martin Häcker

@ghost

ghost commented May 3, 2009

-hash is broken on CPArray

I spoke before I understood what I was talking about.

From the documentation for hash:

@"If two objects are equal (as determined by the isEqual: method), they must have the same hash value. This last point is particularly important if you define hash in a subclass and intend to put instances of that subclass into a collection."

Our implementation of hash does not behave this way. Perhaps we should consider changing our hash to become address, and re-implementing hash the Cocoa way?

by Ross Boucher

maport commented May 5, 2009

See also issue #66 which is related to this.

Member

tolmasky commented Jun 7, 2009

I've taken the first steps to remedy this by replacing all current instances of -hash with the new -UID method, so that when we change hash in the future, it won't break anything.

imrabti commented Mar 28, 2011

This Must be fixed.

@boucher boucher was assigned Apr 2, 2012

Contributor

aparajita commented Apr 9, 2012

All instances of -hash have been replaced by -UID, do we close this issue or change the -hash implementation?

Member

tolmasky commented Apr 9, 2012

So:

  1. We should change -hash at some point.
  2. The problem isn't just internal to Capp, it may be that others are using -hash like -UID currently as well (so at the very least we should announce something or maybe put a log in there warning or something -- maybe we should ask people on the groups if they're using it etc).
  3. It may also be worth changing only after we actually make use of -hash like in the correct implementation of CPSet or something.
Contributor

aparajita commented Apr 9, 2012

Okay, can you please ask about #2 on the list since you understand the issues well? Then we will know better what the ramifications of the change will be.

cappbot commented May 9, 2012

Assignee: boucher. Milestone: 1.0. Labels: #needs-review, Foundation, bug. What's next? This issue is pending an architectural or implementation design decision and should be discussed or voted on.

Contributor

ahankinson commented Feb 23, 2013

Can I have an update on the status of this issue? It seems like it has mostly been addressed and/or circumvented for now.

Contributor

aparajita commented Feb 23, 2013

We basically have decided to hold off on a decision. It's still Someday.

Contributor

daboe01 commented Mar 6, 2015

given that a lot of code has been written since 2009, i would suggest closing this issue and documenting the current behaviour.

@aparajita, any objections?

Contributor

aparajita commented Mar 7, 2015

@daboe01 Go ahead and close it, but document the problem with the current implementation compared to Cocoa.

Contributor

daboe01 commented May 7, 2015

#fixed

cappbot commented May 7, 2015

Assignee: boucher. Milestone: 1.0. Labels: #fixed, Foundation, bug. What's next? This issue is considered successfully resolved.

@cappbot cappbot added #fixed and removed #needs-review labels May 7, 2015

@cappbot cappbot closed this May 7, 2015

cappbot commented May 7, 2015

Assignee: boucher. Milestone: 1.0. Labels: #fixed, Foundation, bug. What's next? This issue is considered successfully resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment