On Tue, 19 Jul 2022 15:48:09 GMT, Andy Goryachev <[email protected]> wrote:
>> - added missing hashCode() methods > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - 8289389: review comments > - Merge remote-tracking branch 'origin/master' into 8289389.hash > - 8289389: minimize the impact of collision > - 8289389: toExternalForm() > - 8289389: implemented missing hashCode() I really must wonder at some of these changes. They seem... unnecessary. Just because equals is overridden, doesn't mean hashCode must be as well, especially not if the objects involved are mutable -- it might in fact be better to leave it alone or return a constant value from it (or only use immutable fields). For example, let's take `FXMLLoader`. I put it in a HashMap. I modify its location. With a traditionally implemented hashCode that checks the same fields as equals, the object will now fail HashMap contains check, even though I am actually using the same instance. If we had left the default hashCode inplace, it would be found correctly (as the identity hash code will be unchanged). However, if we had used a different instance, then the default identity hashCode would not work either. Returning a constant from hashCode then would "solve" the problem again (at the expense of poor performance in large hash maps). TLDR; don't use mutable fields in `hashCode`, if there are only mutable fields, return a constant. Or turn off these too broad warnings that don't really grasp the full impact. I know this is considered a bit of a "holy grail", but it fails in the face of mutable objects that it might be better not to implement hashCode at all and give the wrong impression. ------------- PR: https://git.openjdk.org/jfx/pull/821
