Bug - Fixed hashCode general contract violations

[Catch-22]

I noticed there's two places we're overriding equals() and not overriding the hashCode() method too.

Equal objects must have equal hash codes, so I've implemented fairly basic hashing algorithms for the two classes that needed it (request.UseSkillRequest and textui.parsetree.Value) in order to satisfy the contract conditions.

See patch.

 

[lostcalpolydude]

Only two places? I feel like NetBeans points that out in more places, but I only very recently learned what that meant. I'll take a look at the patch.

 

[Catch-22]

Only two places?

I noticed two, there may be more

I'm in a bit of a bug hunting mood, as I feel like some of the performance issues with KoLmafia could be gradually improved if we keep chipping away at things like this.

Expect to see more stuff like this pop up, if that's okay

 

[lostcalpolydude]

11585

 

[Catch-22]

Looks like there were a lot more than just the ones I found, thanks!

Did Netbeans point those out to you? I don't use it, but that seems useful.

 

[lostcalpolydude]

I did a search for "boolean equals(" and checked out all the results. With the warning that NetBeans adds, it takes maybe a second too look at that section and move on if hashCode() is already there.

It seems like there should be a way to bring up all instances of a warning, but I haven't found it.

 

[Rinn]

You can use static code analysis to locate coding warnings like this. Last time I ran FindBugs a couple months ago it found a ton, but I never got around to fixing them.

Just be careful, sometimes the solution to a static code analysis warning may not be as obvious as you think, so if there's anything questionable you should ping everyone before making a bunch of code changes just to be safe. I also wouldn't make a bunch of unrelated fixes in the same submit even if they're small, just to make it easier to track down if they cause any unintended issues.

 

[xKiv]

Weren't there problems last time this was done (with the same rationale - "overriden equals() must have overriden hashCode()")? There are several different meanings of "equal" and, iirc, some places in the code rely (relied?) on hashCode being different for different objects (because it relied on HashMap differentiating between them, or something like that). I think adding hashCode back then resulted in "mysterious" problems when applying some commands to several items (or batched commands, which end up going to the same code path - again, iirc). I don't remember how that was fixed, but I would be careful ...

 

[Catch-22]

Well, you can satisfy the contract conditions by simply returning 0 for the hashCode. It just makes sorting through the hash tables a lot slower, I believe.

If there's a lot of hash collisions and it's causing performance issues then the hashing algorithm just needs to be tweaked for that particular class. For the handful of classes I looked at though, lost did a decent job at implementing hashing algorithms that wouldn't collide.

Edit: I noticed yesterday that in some places, such as anonymous records, hashCode is being used to uniquely identify the record (because it has no record name). You might be referring to something like that? I posted a patch yesterday which disambiguates the method being used for that purpose, which is either being considered or has been dismissed. Although, there's possibly a better way to uniquely identify a record that doesn't involve referring to it's hashCode.