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:40:43 UTC 2020
Thanks Harold!
Lois
On 8/3/2020 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