RFR(S): JDK-8247938: Change various JVM enums like LinkInfo::AccessCheck and Klass::DefaultsLookupMode to enum class
Lois Foltan
lois.foltan at oracle.com
Mon Aug 3 15:12:12 UTC 2020
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