On Tue, 19 Apr 2022 14:23:07 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:
>> Daniel Jeliński has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - add more factory methods, update copyright
>> - return DEFAULT also when user constraints are null
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 94:
>
>> 92: * @return a SSLAlgorithmConstraints instance
>> 93: */
>> 94: static AlgorithmConstraints forSocket(SSLSocket socket,
>
> I like the idea to use a static method for the construction. What do you
> think if the same coding style is used for other constructors, by making them
> private and add forSocket() methods accordingly? For example, changing the
> following constructor to a private method.
>
> SSLAlgorithmConstraints(SSLEngine engine, String[] supportedAlgorithms,
> boolean withDefaultCertPathConstraints) {
it won't change the performance in any way - when `supportedAlgorithms` are
set, we always need to create a new object. But you're right, it's inconsistent
to offer both constructors and factory methods in the same class. Changed.
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 118:
>
>> 116: if (userSpecifiedConstraints == null) {
>> 117: return withDefaultCertPathConstraints ? DEFAULT :
>> DEFAULT_SSL_ONLY;
>> 118: }
>
> It looks like a partial duplicate of nullIfDefault(). What do you think if
> merging the logic into nullIfDefault()? Or even merging nullIfDefault()
> logic into the getUserSpecifiedConstraints() method? New parameters may be
> required for the getUserSpecifiedConstraints() methods.
I'm not a big fan of modifying `getUserSpecifiedConstraints`; that method has 3
`return` clauses. But you're right, there was some code duplication that is
removed now.
-------------
PR: https://git.openjdk.java.net/jdk/pull/8199
