RFR 8215644: Clean up globalDefinitions_<compiler>.hpp

[Harold David Seigel]

Hi Kim,

Thanks for looking at this.

Please review this updated webrev:

http://cr.openjdk.java.net/~hseigel/bug_8215644.2/webrev/index.html

The only difference from the previous webrev is between lines 1100 - 
1110 in the modified globalDefinitions.hpp file.  The change includes 
removing the two-arg build_u2_from() function as suggested below.

I also created a new RFE, JDK-8216010 
<https://bugs.openjdk.java.net/browse/JDK-8216010>, to change the 
callers of build_u2_from() and remove it.

Thanks, Harold

On 12/27/2018 5:28 PM, Kim Barrett wrote:
>> On Dec 27, 2018, at 5:21 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>
>>> On Dec 20, 2018, at 11:58 AM, Harold David Seigel <harold.seigel at oracle.com> wrote:
>>>
>>> Hi,
>>>
>>> Please review this fix for JDK-8215644.  The fix deletes old code and moves misplaced code as described in the bug.
>>>
>>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8215644/webrev/index.html
>>>
>>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8215644
>>>
>>> The fix was regression tested by running Mach5 tiers 1 and 2 tests and builds on Linux-x64, Windows, and Mac OS X and by running JCK-12 Lang and VM tests on Linux-x64.
>>>
>>> Thanks, Harold
> I forgot to say, looks good.
>
>> ------------------------------------------------------------------------------
>> src/hotspot/share/utilities/globalDefinitions.hpp
>> 1104 inline u2 build_u2_from( u1 c1, u1 c2 ) {
>>
>> Other than the build_u2_from(u1* p) overload a few lines later, there
>> are no callers of this two-arg overload.  So this one could be removed
>> by inlining the implementation into the one-arg overload.
>>
>> Also, the comment describing these as building values using the
>> conventions used in class files has been lost.
>>
>> But the best cleanup would be to also entirely remove build_u2_from()
>> and change its callers to instead call Bytes::get_Java_u2().  There
>> are 14 callers.  Feel free to make that a separate cleanup RFE, if you
>> want to avoid expanding the scope of this change.
>>
>> I note that you also removed this comment:
>> 1131 // On the 386, this could be realized with a simple address cast.
>> which is good, since that comment seems to be just plain wrong.
>>
>> ------------------------------------------------------------------------------
>