On Wed, 7 Aug 2024 19:13:32 GMT, Martin Balao <mba...@openjdk.org> wrote:
>> src/java.base/share/classes/java/security/Security.java line 211:
>>
>>> 209: }
>>> 210:
>>> 211: private static void reset(LoadingMode mode) {
>>
>> The method here looks like there is a chance that `props` does not get
>> assigned. I know when the main file is loaded the mode is OVERRIDE, so this
>> will not actually happen. Do you want to add a comment on this?
>>
>> Or, maybe we can assign `props` at the beginning and use APPEND mode when
>> loading the main file?
>
> Ok, we will change this to initialize `props` statically and then, if needed
> because of an OVERRIDE, do a `clear` of the properties map.
Done in 580d34a63b5d3b6c7d2323413f338527db2d9acd.
>> src/java.base/share/classes/sun/security/util/PropertyExpander.java line 72:
>>
>>> 70: } catch (ExpandException e) {
>>> 71: // should not happen
>>> 72: throw new RuntimeException("unexpected expansion error:
>>> when " +
>>
>> Is an `AssertionError` better here?
>
> Yes, we can use an `AssertionError` here
Done in 580d34a63b5d3b6c7d2323413f338527db2d9acd.
>> src/java.base/share/conf/security/java.security line 33:
>>
>>> 31: #
>>> 32: # The special "include" property can be defined one or multiple times in
>>> 33: # this file with a filesystem path value. The effect of each definition
>>
>> "this file". Is this precise? As described below, It can also appear in
>> included files and the file "java.security.properties" points to.
>>
>> Maybe not an issue since no one will be confused. You decide whether to make
>> any update.
>
> I don't see "this file" as strictly necessary given the context so we can
> remove it.
Done in 580d34a63b5d3b6c7d2323413f338527db2d9acd.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1707804517
PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1707807676
PR Review Comment: https://git.openjdk.org/jdk/pull/16483#discussion_r1707807978
