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:45:24 UTC 2020
Thanks Coleen!
Lois
On 8/3/2020 11:44 AM, coleen.phillimore at oracle.com wrote:
> +1. It was a good suggestion.
> Coleen
>
> On 8/3/20 11:39 AM, Harold Seigel wrote:
>> 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