[13] RFR: 8213416: Replace some enums with static const members in hotspot/compiler

[Rahul Raghavan]

Hi,

With reference to below email thread, request help to confirm next steps 
for JDK-8213416.

So may I go ahead with webrev changes related to hotspot/compiler for 
8213416 ?
<webrev.01> - http://cr.openjdk.java.net/~rraghavan/8213416/webrev.01/

(also will add similar hotspot/runtime related details in JBS comments 
for JDK-8223400)

Thanks,
Rahul



On 16/05/19 3:26 PM, Rahul Raghavan wrote:> Hi,
 >
 > Thank you David for review comments.
 >
 > I will kindly request help from Magnus to reply for the main questions.
 >
 >
 > Sharing some notes, related links -
 > - 8211073: Remove -Wno-extra from Hotspot
 >      https://bugs.openjdk.java.net/browse/JDK-8211073
 > - Discussions in earlier thread -
 > 
https://mail.openjdk.java.net/pipermail/hotspot-dev/2018-September/034314.html 

 >
 >
 > So understood -Wextra do help in catching valid/useful warnings also,
 > but along with some too strict ones like "enumeral and non-enumeral type
 > in conditional expression" type warnings.
 >
 > Extracts from 8211073 JBS comments from Magnus regarding the
 > 'enum-warning' -
 > "... If you think that gcc is a bit too picky here, I agree. It's not
 > obvious per se that the added casts improve the code. However, this is
 > the price we need to pay to be able to enable -Wextra, and *that* is
 > something that is likely to improve the code."
 >
 >
 > Thanks,
 > Rahul
 >




 > On 16/05/19 11:13 AM, David Holmes wrote:
 >> This all seems like unnecessary churn to me - is any of this code
 >> actually wrong? can we not just disable this particular warning? is
 >> there any point using "static const" when we should be aiming to use
 >> C++11 constexpr in the (not too distant?) future?
 >>
 >> Converting from enums to unrelated ints seems a big step backwards in
 >> software engineering terms. :(
 >>
 >> Cheers,
 >> David
 >> -----
 >>



On 16/05/19 12:03 PM, Rahul Raghavan wrote:
> Hi,
> 
> Thank you Vladimir for review comments.
> 
>>> 4) _lh_array_tag_obj_value, _lh_instance_slow_path_bit -
>>> [open/src/hotspot/share/oops/klass.cpp]
>>> ..........
>>
>> I am okay with it but Runtime group should agree too - it is their code.
>>
> Yes, I missed that it is Runtime code.
> 
> Please note plan is to handle only the hotspot/compiler part of the 
> requirement changes in JDK-8213416.
> As per earlier JBS comments new JDK-8223400 was created to cover the 
> requirements in hotspot/runtime.
> So may I suggest moving the above runtime change requirement details to 
> JDK-822340;
> and use only the balance changes, as in below updated webrev, here for 
> 8213416.
> 
> <webrev.01> - http://cr.openjdk.java.net/~rraghavan/8213416/webrev.01/
> 
> 
> Thanks,
> Rahul