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

[Eirik Bjørsnøs]

On Wed, 4 Dec 2024 16:46:14 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

>> 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
>
> Good :-)
> 
> If you end up keeping the `JAR_CHECKING_ENABLED` name, then introducing a local variable for the evaluation of the property may be useful. It should also fit better with the comment you introduced: 
> 
> 
> String p = props.getProperty("sun.misc.URLClassPath.disableJarChecking");
> // JAR check is disabled by default and will be enabled only if the "disable JAR check"
> // system property has been set to "false".
>  boolean jarCheckingDisabled = "false".equals(p);
> JAR_CHECKING_ENABLED = !jarCheckingDisabled;
> 
> 
> But in my opinion the above just makes it explicit that we flip a boolean here just to flip it back again on the use site.

> 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

Yes, agree that if we could start afresh, it would be better to disable checking using "enableJarChecking=false".  And yes, that ship has sailed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22545#discussion_r1869932859