RFR: JDK-8284112 Minor cleanup could be done in javax.crypto

[Mark Powers]

On Wed, 13 Apr 2022 06:31:51 GMT, Bradford Wetmore <wetmore at openjdk.org> wrote:

>> JDK-8284112 Minor cleanup could be done in javax.crypto
>
> src/java.base/share/classes/javax/crypto/CipherInputStream.java line 159:
> 
>> 157:         try {
>> 158:             ofinish = cipher.update(ibuffer, 0, readin, obuffer, ostart);
>> 159:         } catch (IllegalStateException e) {
> 
> This is another one of those I would probably leave alone, just so it's clear what should be done.

Keeping the original.

> src/java.base/share/classes/javax/crypto/CipherSpi.java line 29:
> 
>> 27: 
>> 28: import java.nio.ByteBuffer;
>> 29: import java.security.*;
> 
> This is another one that some people will complain about.  I personally prefer this style, others prefer everything written out as long as it's not "too many."

Keeping the change for now.

> src/java.base/share/classes/javax/crypto/CryptoPermission.java line 437:
> 
>> 435:             // may be the best try.
>> 436:             return this.algParamSpec.equals(algParamSpec);
>> 437:         } else return !this.checkParam;
> 
> Please use the standard coding format.
> 
>     } else { 
>         return !this.checkParam;
>     }
> 
> or
> 
>     } 
>     
>     return !this.checkParam;

Using the first format.

> src/java.base/share/classes/javax/crypto/CryptoPermissions.java line 158:
> 
>> 156: 
>> 157:     /**
>> 158:      * Checks if this object's PermissionCollection for permissions
> 
> Just FYI and not for this review, but this class should really be updated to use proper javadoc comment style, which is to tag all objects with < code >PermissionCollection< /code >

No change required.

> src/java.base/share/classes/javax/crypto/ExemptionMechanism.java line 142:
> 
>> 140:      * @see java.security.Provider
>> 141:      */
>> 142:     public static ExemptionMechanism getInstance(String algorithm)
> 
> Others might disagree with this comment, but I would encourage you not to make these changes throughout your PR.  (@jddarcy ?) 
> 
> Even though a static method is implicitly final and IJ is correct, the final keyword does prevent subclasses from inadvertently using the same method signature.  
> 
> Once this is resolved, I can approve.

Keeping the original here and elsewhere.

> src/java.base/share/classes/javax/crypto/SealedObject.java line 449:
> 
>> 447: final class extObjectInputStream extends ObjectInputStream {
>> 448:     extObjectInputStream(InputStream in)
>> 449:         throws IOException {
> 
> This is another "I probably wouldn't do that..."  
> 
> It's nice to know what kind of IOExceptions you might get, so leaving StreamCorruptedException is ok.

Keeping the original. Still, why would IJ suggest removing this unless it had already figured out the exception could never happen?

> src/java.base/share/classes/javax/crypto/spec/DESKeySpec.java line 49:
> 
>> 47:      * Weak/semi-weak keys copied from FIPS 74.
>> 48:      *
>> 49:      * "...The first 6 keys have duals different from themselves, hence
> 
> Suggest you leave this alone as this is a direct quote from FIPS 74 (page 10).

Keeping the original.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8214