RFR: 8345139: Fix bugs and inconsistencies in the Provider services map [v5]

[Martin Balao]

On Thu, 6 Mar 2025 06:10:09 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> Francisco Ferrari Bihurriet has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Clear ServicesMap fields in the declared order
>>   
>>   Constructors assign the fields in the same order.
>
> src/java.base/share/classes/java/security/Provider.java line 1486:
> 
>> 1484:      * case, Properties map entries are synchronized. In the latter, Properties
>> 1485:      * map entries are not modified.
>> 1486:      */
> 
> I am not too sure I get this comment block. Judging from the impl, this enum seems to be used to indicate whether the Properties map is updated. The part about ServiceMap is especially confusing. Is this enum for Properties map or ServicesMap? In addition, instead of stating "an entry that is not a service, alias or attribute.", can we just state the remaining types? Is "In the former case" refer to the UPDATE? In that paragraph. there is only one case. Lastly, there is only 2 values to this enum, can't this be replaced with a boolean?

Yes, we can replace a 2-values enum with a boolean if you have a strong preference. We have done this before. I still think that an enum and a comment may help the reader, especially because uses are in multiple private and undocumented methods. This enum allows us to have the documentation in a single place.

As for the comment itself, UPDATE indicates that the change in the outer (properties) map has to be done. SKIP indicates that the outer map change should be skipped: it was either done automatically when the ServicesMap was updated or should not be done because there was an error.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r2025839530