Bug 353593

Comment 1 Daniel Thomas 2011-08-02 10:30:17 EDT

Created attachment 200720 [details]
Patch to add equals and hashCode methods

Comment 2 Daniel Thomas 2011-08-04 07:52:24 EDT

Created attachment 200906 [details]
Test to check equals and hashCode work

Relies on https://bugs.eclipse.org/bugs/attachment.cgi?id=200904

Comment 3 Kirk Beitz 2011-08-05 15:41:58 EDT

the patch has been merged and nominally tested in our local source base.

the test case has been added instead to org.eclipse.cdt.debug.edc.tests.ScopeTest rather than as a separate regression test.

both are waiting for final review and merge to eclipse edc git repository by committer Ken Ryall.

Comment 4 Kirk Beitz 2011-08-22 12:20:10 EDT

after testing these changes for a bit, we discovered a regression having to do with the fact that the lowAddress and highAddress are mutable, and can cause the hashCode values to be computed differently before and after fixupRanges, and leave the user with HashMap and HashSet containers whose contents contain duplicate entries and eventually pointers to symbols that are stale.

our analysis showed that the most important scope involving hashCode values is compileUnitScope, and so we have created abstract hashCode()/equals() in CompileUnitScope, forcing specific implementers of ICompileUnitScope to provide a hashCode(), which allows ModuleLineEntryProvider to have its hashSet of ICompileUnitScope to behave properly.

please comment if there is a specific need you have for hashCode() for Scope itself.

Comment 5 Daniel Thomas 2011-08-23 09:05:33 EDT

I have a HashMap of IScopes to other things to which scopes are added after being fully parsed so that their high and low addresses will not change. Hence a need for hashCode in Scope.

Comment 6 Kirk Beitz 2011-08-23 18:45:20 EDT

(In reply to comment #5)
> I have a HashMap of IScopes to other things to which scopes are added after
> being fully parsed so that their high and low addresses will not change. Hence
> a need for hashCode in Scope.

for such a case, it still needs to be a rule that hashCode()/equals() work only with immutable members, and lowAddress/highAddress do not qualify, even if you intend only to use them after they won't be changed.

my best recommendation would be to override Scope (and whatever other extensions you need to place in the HashMaps you are using) with extensions that would use constructors that would copy the Scope in which you have finalized the low and high addresses.  make these classes implementors of an interface named something like IFinalizedScope that is an extension of IScope, and in which you can implement hashCode()/equals() in a way that you know will rely on values that would be immutable.

whatever is done, it has to guarantee that nothing will confuse one scope for another just because some immutable value has been changed despite attempts/efforts to conventionalize the method that the objects are used/stored.

Comment 7 Daniel Thomas 2011-08-31 08:22:11 EDT

Created attachment 202508 [details]
Patch implementing hashCode and equals and only using immutable fields for hashCode

Only hashCode is required to only use immutable members and it does not have to return a unique value - just a pretty unique value. Hence by making parent and name immutable it is possible to write a hashCode in Scope which works fine and the value of which does not vary.

Comment 8 Kirk Beitz 2011-09-01 21:26:15 EDT

(In reply to comment #7)
> Created attachment 202508 [details]
> Patch implementing hashCode and equals and only using immutable fields for
> hashCode
> 
> Only hashCode is required to only use immutable members and it does not have to
> return a unique value - just a pretty unique value. Hence by making parent and
> name immutable it is possible to write a hashCode in Scope which works fine and
> the value of which does not vary.

one of the problems we had originally been facing that led us to completely separating the compileUnitScope and below from the scope from above was directly related to the lineEntries information changing and cashing a visited hashSet from behaving properly.

so adding lineEntries back into the equals function raises a huge red flag.

i will agree that, in principal & theory, what gets compared in equals() doesn't have to be what gets used for calculation in hashCode() ... but from the very real pragmatic standpoint of testing, the regressions we hit when trying to use variations of hashCode()/equals() before finally settling on what's in eclipse edc git repository now make me wary to wade back in with this commit.

Comment 9 Jonah Graham 2019-12-30 18:39:32 EST

(this is part of a batch change)

The Eclipse CDT EDC (https://wiki.eclipse.org/CDT/cdt-debug-edc) is now obsolete and has not had any active development since 2011. Therefore the still open bugs are being marked as wontfix.

The git repo for the project still exists for posterity at https://git.eclipse.org/c/cdt/org.eclipse.cdt.edc.git/