RFR(S): JDK-8247938: Change various JVM enums like LinkInfo::AccessCheck and Klass::DefaultsLookupMode to enum class
Harold Seigel
harold.seigel at oracle.com
Mon Aug 3 15:39:34 UTC 2020
Hi Lois,
The new changes look good!
Thanks, Harold
On 8/3/2020 11:12 AM, Lois Foltan wrote:
>
>
>
>
> On 8/1/2020 5:47 AM, Kim Barrett wrote:
>>> On Jul 31, 2020, at 1:55 PM, Lois Foltan <lois.foltan at oracle.com>
>>> wrote:
>>>
>>> Please review the webrev to change the following unscoped
>>> enumeration declarations to scoped enumerations.
>>>
>>> Klass::DefaultsLookupMode
>>> Klass::OverpassLookupMode
>>> Klass::StaticLookupMode
>>> Klass::PrivateLookupMode
>>> LinkInfo::AccessCheck
>>>
>>> With C++11/14 enum class provides the added type safety to prevent 0
>>> (integral) or false (boolean) to be implicitly converted to
>>> AccessCheck::needs_access_check for example.
>>>
>>> open webrev
>>> at:http://cr.openjdk.java.net/~lfoltan/bug_jdk8247938.0/webrev/
>>> <http://cr.openjdk.java.net/~lfoltan/bug_jdk8247938.0/webrev/index.html>
>>>
>>> bug link: https://bugs.openjdk.java.net/browse/JDK-8247938
>>>
>>> Testing: hs-tier1 & 2 complete, hs-tier3-6 in progress
>>>
>>> Thanks,
>>> Lois
>> Looks good.
>>
>> For what it's worth, I didn't notice any cases here that seemed like
>> they
>> would significantly benefit from C++20 "using enum":
>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html
>> (Though it could have been used to signficantly reduce the size of
>> the change.)
>>
>> However, it does seem like some of the enumerators might benefit from
>> a name
>> change, since they are now referred to by names that are qualified by
>> the
>> enum type name. For example, instead of this:
>>
>> 199 enum class DefaultsLookupMode { find_defaults, skip_defaults };
>> 200 enum class OverpassLookupMode { find_overpass, skip_overpass };
>> 201 enum class StaticLookupMode { find_static, skip_static };
>> 202 enum class PrivateLookupMode { find_private, skip_private };
>>
>> maybe use this:
>>
>> 199 enum class DefaultsLookupMode { find, skip };
>> 200 enum class OverpassLookupMode { find, skip };
>> 201 enum class StaticLookupMode { find, skip };
>> 202 enum class PrivateLookupMode { find, skip };
>>
>> since (for example) DefaultsLookupMode::find is obviously distinct from
>> StaticLookupMode::find, with the numerator suffixes being redundant.
>>
>> Similarly, in class LinkInfo, perhaps:
>> enum class AccessCheck { required, skip };
>>
>> But that sort of thing can be done separately, assuming it's deemed
>> desirable. Getting the benfit of the improved type safety is the
>> important
>> part of this change.
>>
>>
>
> Thank you Kim for the review! I do like your suggestion of changing
> the names of the enumerators. New webrev to include that renaming as
> part of this change.
>
> New webrev at:
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8247938.1/webrev
>
> Retesting hs-tier1-3.
>
> Thanks,
> Lois
More information about the hotspot-runtime-dev
mailing list