On Fri, 26 Aug 2022 14:14:13 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> Look at implementation and figure out what happens if you do:
>>
>>
>> computeHash("SHA-1") = someHash;
>> computeHash("SHA-256") = ...?
>>
>>
>> The caching method should actually check the algorithms match.
>>
>> Not a bug at this point, since only use SHA-256 today, but this is a
>> landmine ready to fire.
>>
>> Additional testing:
>> - [x] Linux x86_64 release, `java/lang/module` tests
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Touchups
src/java.base/share/classes/jdk/internal/module/ModuleReferenceImpl.java line
70:
> 68: // ConcurrentMap<String,byte[]>.
> 69: //
> 70: // For correctness under concurrent updates, we need to wrap the
> fields
The comment "If there is a significant ..." reads more like a "TODO" and can be
removed.
src/java.base/share/classes/jdk/internal/module/ModuleReferenceImpl.java line
75:
> 73: }
> 74: final byte[] hash;
> 75: final String algorithm;
Have you tried using a record? Also can you move the declaration of cachedHash
to after the declaration of the record.
src/java.base/share/classes/jdk/internal/module/ModuleReferenceImpl.java line
154:
> 152: CachedHash ch = cachedHash;
> 153: if (ch != null) {
> 154: if (ch.algorithm.equals(algorithm)) {
The nested if looks a bit strange here, you can change to:
if (ch != null & ch.algorithm.equals(algorithm))
return ch.hash;
-------------
PR: https://git.openjdk.org/jdk/pull/10044
