RFR: 8341551: Revisit jdk.internal.loader.URLClassPath.JarLoader after JEP 486 [v2]

[Roger Riggs]

On Wed, 4 Dec 2024 16:35:19 GMT, Jaikiran Pai <jpai at 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