On Thu, 25 Aug 2022 18:44:54 GMT, Mark Powers <mpow...@openjdk.org> wrote:
>> src/java.base/share/classes/sun/security/jca/ProviderList.java line 129:
>>
>>> 127: int j = 0;
>>> 128: for (ProviderConfig config : providerList.configs) {
>>> 129: if
>>> (!Objects.requireNonNull(config.getProvider()).getName().equals(name)) {
>>
>> This is an unusual usage of `Objects.requireNonNull`. Is a null provider
>> ever expected here? I don't see why this is better, the prior code will also
>> throw NPE. Replacing `== false` with `!` is ok though.
>>
>> Same comment on other cases in this file.
>
> IntelliJ seems to think it could. The point of requireNonNull is that you
> control when an exception is thrown, and sooner rather than later is better.
> This code appears to have been working fine for a long time, so maybe NPE
> can't happen in practice. I'm fine with reverting this requireNonNull change
> here and elsewhere if you think it is unnecessary.
Right, but in this case I think if an NPE is ever thrown it would be considered
a bug in the JDK because an unexpected RuntimeException would be thrown. I
think requireNonNull is used more in cases where caller input is being
validated and null is not valid. I find this code less readable. There are lots
of cases in the JDK code where some object could theoretically be null, but it
would be a bug if it was. If it was a normal case for a provider to sometimes
be null here, then I would expect this code to check for null and handle it.
@valeriep is more familiar with this code, so I would also like her feedback on
these changes to use requireNonNull.
-------------
PR: https://git.openjdk.org/jdk/pull/9972
