On Wed, 5 Jul 2023 12:31:09 GMT, Daniel Jeliński <djelin...@openjdk.org> wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add even more cases and tidy up
>
> src/java.base/share/classes/java/security/spec/ECFieldF2m.java line 235:
>
>> 233: public int hashCode() {
>> 234: int value = m << 5;
>> 235: // consider simplifying using Objects.hashCode(rp) after
>> JDK-8015417
>
> Could you explain this comment? Why does it apply here, and not to other
> methods like CodeSigner.hashCode?
You are absolutely right: adding that comment, while actively disregarding
concerns described in it elsewhere, warrants some explanation.
While working on this and the related PRs, I discovered JDK-8015417. hashCode
and, to a lesser extent, equals in ECFieldF2m seemed lean and
performance-sensitive so as to apply the smarts of JDK-8015417. Frankly,
performance sensitivity was my guess; anyway, I kept the original behaviour for
that site.
Since then, I learned that JDK-8015417 is quite complicated and well beyond my
level of understanding of how JVM works. Until I have clearer understanding of
the effects of that issue on this and similar refactoring efforts, I won't
integrate this or any other PRs where using the java.util.Objects static
methods is a primary goal.
That said, I'll remove the comment and refactor that `?:` conditional to use
`Objects.equals(Object, Object)`, to be consistent with the rest of this PR.
FWIW, we've accidentally started discussion on those effects in another PR:
https://github.com/openjdk/jdk/pull/14752#pullrequestreview-1511190453. It
might as well have been this PR, but that PR was first. While either PR is
good, let's keep it confined and organized. So, please follow that discussion,
if you're interested.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14738#discussion_r1253214761
