On Wed, 4 Dec 2024 16:35:19 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 89:
>>
>>> 87: // JAR check is disabled by default and will be enabled only if
>>> the "disable JAR check"
>>> 88: // system property has been set to "false".
>>> 89: JAR_CHECKING_ENABLED = p != null && p.equals("false");
>>
>> I throw `BooleanArithmeticOverflowException` :-)
>>
>> I think this would be easier to follow if the static field name was aligned
>> with the property name itself. It's easier if they have the same
>> interpretation.
>>
>> So you could keep `DISABLE_JAR_CHECKING` and just do
>>
>> `DISABLE_JAR_CHECKING = !"false".equals(p)`
>>
>> and then just `if (DISABLE_JAR_CHECKING)` below.
>>
>> If you don't like that, at least consider that the above can be simplified
>> to `"false".equals(p)`
>
> Hello Eirik, I have received similar inputs in other places when discussing
> this change. I myself had to think a few times on which naming to follow here
> to make it easier to understand the code. I'll consider your and other inputs
> and come back to this tomorrow afresh.
We're stuck with the property name for compatibility, and the usage within the
class is fairly limited.
Generally, it is easier to understand the behavior having a feature that is
enabled not a disable that is disabled.
$0.02
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22545#discussion_r1869917036
