RFR: 8205459: Rename Access API flag decorators

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8205459: Rename Access API flag decorators

Kim Barrett
Please review these name and behavioral changes for some Access API
decorators.  We want to recategorize these as orthogonal boolean
flags.  Specifically,

OOP_NOT_NULL => IS_NOT_NULL
AS_DEST_NOT_INITIALIZED => IS_DEST_UNINITIALIZED
IN_HEAP_ARRAY => IN_ARRAY

In addition:

Remove OOP_DECORATOR_MASK.

Change IN_ARRAY (formerly IN_HEAP_ARRAY) to no longer imply IN_HEAP.
Note that all of the places where IN_HEAP_ARRAY was being specified
already also specified IN_HEAP.

Some cleanups, such as renaming local variables from "on_array" to
"is_array", for consistency with the associated decorator name.

To aid reviewing, separate webrevs for each renaming are provided.

Each of these renamings involves a lot of mechanical text replacement,
with some further manual changes.  Separate webrevs are also provided
for the mechanical and manual parts of each renaming.

CR:
https://bugs.openjdk.java.net/browse/JDK-8205459

Testing:
Mach5 tier1,2,3,hs-tier4,5.

Webrevs:

The entire set of changes:

http://cr.openjdk.java.net/~kbarrett/8205459/open.00/

That's made up of the following three changes, applied in order:

(1) Renaming OOP_NOT_NULL => IS_NOT_NULL

http://cr.openjdk.java.net/~kbarrett/8205459/rename_oop_not_null/

which consists of these:
http://cr.openjdk.java.net/~kbarrett/8205459/rename_oop_not_null_mechanical/
http://cr.openjdk.java.net/~kbarrett/8205459/rename_oop_not_null_manual/

The mechanical part was made using the following bash commands,
executed from src/hotspot:

find . -type f -name "*.[ch]pp" \
  -exec grep -q OOP_NOT_NULL {} \; -print \
  | xargs sed -i 's/OOP_NOT_NULL/IS_NOT_NULL/'

sed -i '/const DecoratorSet OOP_DECORATOR_MASK/d' share/oops/accessDecorators.hpp

find . -type f -name "*.[ch]pp" \
  -exec grep -q OOP_DECORATOR_MASK {} \; -print \
  | xargs sed -i 's/OOP_DECORATOR_MASK/IS_NOT_NULL/'

(2) Renaming AS_DEST_NOT_INITIALIZED to IS_DEST_UNINITIALIZED

http://cr.openjdk.java.net/~kbarrett/8205459/rename_as_dest_not_initialized/

which consists of these:
http://cr.openjdk.java.net/~kbarrett/8205459/rename_as_dest_not_initialized_mechanical/
http://cr.openjdk.java.net/~kbarrett/8205459/rename_as_dest_not_initialized_manual/

The mechanical part was made using the following bash command,
executed from src/hotspot:

find . -type f -name "*.[ch]pp" \
  -exec grep -q AS_DEST_NOT_INITIALIZED {} \; -print \
  | xargs sed -i 's/AS_DEST_NOT_INITIALIZED/IS_DEST_UNINITIALIZED/'

(3) Renaming IN_HEAP_ARRAY to IN_ARRAY

http://cr.openjdk.java.net/~kbarrett/8205459/rename_in_heap_array/

which consists of these:
http://cr.openjdk.java.net/~kbarrett/8205459/rename_in_heap_array_mechanical/
http://cr.openjdk.java.net/~kbarrett/8205459/rename_in_heap_array_manual/

The mechanical part was made using the following bash commands,
executed from src/hotspot:

find . -type f -name "*.[ch]pp" \
  -exec grep -q IN_HEAP_ARRAY {} \; -print \
  | xargs sed -i 's/IN_HEAP_ARRAY/IS_ARRAY/'

cd cpu
find . -type f -name "*.[ch]pp" \
  -exec grep -q "bool on_array = " {} \; -print \
  | xargs sed -i 's/on_array/is_array/'


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8205459: Rename Access API flag decorators

Stefan Karlsson
Looks good.

StefanK

On 2018-06-21 10:25, Kim Barrett wrote:

> Please review these name and behavioral changes for some Access API
> decorators.  We want to recategorize these as orthogonal boolean
> flags.  Specifically,
>
> OOP_NOT_NULL => IS_NOT_NULL
> AS_DEST_NOT_INITIALIZED => IS_DEST_UNINITIALIZED
> IN_HEAP_ARRAY => IN_ARRAY
>
> In addition:
>
> Remove OOP_DECORATOR_MASK.
>
> Change IN_ARRAY (formerly IN_HEAP_ARRAY) to no longer imply IN_HEAP.
> Note that all of the places where IN_HEAP_ARRAY was being specified
> already also specified IN_HEAP.
>
> Some cleanups, such as renaming local variables from "on_array" to
> "is_array", for consistency with the associated decorator name.
>
> To aid reviewing, separate webrevs for each renaming are provided.
>
> Each of these renamings involves a lot of mechanical text replacement,
> with some further manual changes.  Separate webrevs are also provided
> for the mechanical and manual parts of each renaming.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8205459
>
> Testing:
> Mach5 tier1,2,3,hs-tier4,5.
>
> Webrevs:
>
> The entire set of changes:
>
> http://cr.openjdk.java.net/~kbarrett/8205459/open.00/
>
> That's made up of the following three changes, applied in order:
>
> (1) Renaming OOP_NOT_NULL => IS_NOT_NULL
>
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_oop_not_null/
>
> which consists of these:
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_oop_not_null_mechanical/
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_oop_not_null_manual/
>
> The mechanical part was made using the following bash commands,
> executed from src/hotspot:
>
> find . -type f -name "*.[ch]pp" \
>    -exec grep -q OOP_NOT_NULL {} \; -print \
>    | xargs sed -i 's/OOP_NOT_NULL/IS_NOT_NULL/'
>
> sed -i '/const DecoratorSet OOP_DECORATOR_MASK/d' share/oops/accessDecorators.hpp
>
> find . -type f -name "*.[ch]pp" \
>    -exec grep -q OOP_DECORATOR_MASK {} \; -print \
>    | xargs sed -i 's/OOP_DECORATOR_MASK/IS_NOT_NULL/'
>
> (2) Renaming AS_DEST_NOT_INITIALIZED to IS_DEST_UNINITIALIZED
>
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_as_dest_not_initialized/
>
> which consists of these:
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_as_dest_not_initialized_mechanical/
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_as_dest_not_initialized_manual/
>
> The mechanical part was made using the following bash command,
> executed from src/hotspot:
>
> find . -type f -name "*.[ch]pp" \
>    -exec grep -q AS_DEST_NOT_INITIALIZED {} \; -print \
>    | xargs sed -i 's/AS_DEST_NOT_INITIALIZED/IS_DEST_UNINITIALIZED/'
>
> (3) Renaming IN_HEAP_ARRAY to IN_ARRAY
>
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_in_heap_array/
>
> which consists of these:
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_in_heap_array_mechanical/
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_in_heap_array_manual/
>
> The mechanical part was made using the following bash commands,
> executed from src/hotspot:
>
> find . -type f -name "*.[ch]pp" \
>    -exec grep -q IN_HEAP_ARRAY {} \; -print \
>    | xargs sed -i 's/IN_HEAP_ARRAY/IS_ARRAY/'
>
> cd cpu
> find . -type f -name "*.[ch]pp" \
>    -exec grep -q "bool on_array = " {} \; -print \
>    | xargs sed -i 's/on_array/is_array/'
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8205459: Rename Access API flag decorators

Per Liden
In reply to this post by Kim Barrett
Hi Kim,

On 06/21/2018 10:25 AM, Kim Barrett wrote:

> Please review these name and behavioral changes for some Access API
> decorators.  We want to recategorize these as orthogonal boolean
> flags.  Specifically,
>
> OOP_NOT_NULL => IS_NOT_NULL
> AS_DEST_NOT_INITIALIZED => IS_DEST_UNINITIALIZED
> IN_HEAP_ARRAY => IN_ARRAY
>
> In addition:
>
> Remove OOP_DECORATOR_MASK.
>
> Change IN_ARRAY (formerly IN_HEAP_ARRAY) to no longer imply IN_HEAP.
> Note that all of the places where IN_HEAP_ARRAY was being specified
> already also specified IN_HEAP.
>
> Some cleanups, such as renaming local variables from "on_array" to
> "is_array", for consistency with the associated decorator name.
>
> To aid reviewing, separate webrevs for each renaming are provided.
>
> Each of these renamings involves a lot of mechanical text replacement,
> with some further manual changes.  Separate webrevs are also provided
> for the mechanical and manual parts of each renaming.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8205459
>
> Testing:
> Mach5 tier1,2,3,hs-tier4,5.
>
> Webrevs:
>
> The entire set of changes:
>
> http://cr.openjdk.java.net/~kbarrett/8205459/open.00/

Looks good, just two comments:

src/hotspot/share/oops/accessDecorators.hpp
-------------------------------------------
Looks like the decorator numbers got a bit mixed up:

  [...]
  155 const DecoratorSet AS_RAW                  = UCONST64(1) << 12;
  156 const DecoratorSet AS_NO_KEEPALIVE         = UCONST64(1) << 14;
  [...]
  185 const DecoratorSet IN_HEAP            = UCONST64(1) << 20;
  186 const DecoratorSet IN_NATIVE          = UCONST64(1) << 22;
  [...]
  196 const DecoratorSet IS_ARRAY              = UCONST64(1) << 21;
  197 const DecoratorSet IS_DEST_UNINITIALIZED = UCONST64(1) << 13;
  198 const DecoratorSet IS_NOT_NULL           = UCONST64(1) << 25;
  [...]

Stefan just told me you planned to fix this in a different patch?


src/hotspot/share/oops/access.hpp
---------------------------------
Could we turn this:

  123                                              IS_ARRAY | IS_NOT_NULL |
  124                                              IN_HEAP;

Into a single line, with IN_HEAP first, like:

  123                                              IN_HEAP | IS_ARRAY |
IS_NOT_NULL;


cheers,
Per

>
> That's made up of the following three changes, applied in order:
>
> (1) Renaming OOP_NOT_NULL => IS_NOT_NULL
>
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_oop_not_null/
>
> which consists of these:
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_oop_not_null_mechanical/
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_oop_not_null_manual/
>
> The mechanical part was made using the following bash commands,
> executed from src/hotspot:
>
> find . -type f -name "*.[ch]pp" \
>    -exec grep -q OOP_NOT_NULL {} \; -print \
>    | xargs sed -i 's/OOP_NOT_NULL/IS_NOT_NULL/'
>
> sed -i '/const DecoratorSet OOP_DECORATOR_MASK/d' share/oops/accessDecorators.hpp
>
> find . -type f -name "*.[ch]pp" \
>    -exec grep -q OOP_DECORATOR_MASK {} \; -print \
>    | xargs sed -i 's/OOP_DECORATOR_MASK/IS_NOT_NULL/'
>
> (2) Renaming AS_DEST_NOT_INITIALIZED to IS_DEST_UNINITIALIZED
>
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_as_dest_not_initialized/
>
> which consists of these:
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_as_dest_not_initialized_mechanical/
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_as_dest_not_initialized_manual/
>
> The mechanical part was made using the following bash command,
> executed from src/hotspot:
>
> find . -type f -name "*.[ch]pp" \
>    -exec grep -q AS_DEST_NOT_INITIALIZED {} \; -print \
>    | xargs sed -i 's/AS_DEST_NOT_INITIALIZED/IS_DEST_UNINITIALIZED/'
>
> (3) Renaming IN_HEAP_ARRAY to IN_ARRAY
>
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_in_heap_array/
>
> which consists of these:
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_in_heap_array_mechanical/
> http://cr.openjdk.java.net/~kbarrett/8205459/rename_in_heap_array_manual/
>
> The mechanical part was made using the following bash commands,
> executed from src/hotspot:
>
> find . -type f -name "*.[ch]pp" \
>    -exec grep -q IN_HEAP_ARRAY {} \; -print \
>    | xargs sed -i 's/IN_HEAP_ARRAY/IS_ARRAY/'
>
> cd cpu
> find . -type f -name "*.[ch]pp" \
>    -exec grep -q "bool on_array = " {} \; -print \
>    | xargs sed -i 's/on_array/is_array/'
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8205459: Rename Access API flag decorators

Kim Barrett
In reply to this post by Stefan Karlsson
> On Jun 21, 2018, at 5:30 AM, Stefan Karlsson <[hidden email]> wrote:
>
> Looks good.
>
> StefanK

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8205459: Rename Access API flag decorators

Kim Barrett
In reply to this post by Per Liden
> On Jun 21, 2018, at 5:32 AM, Per Liden <[hidden email]> wrote:
>
> Looks good, just two comments:

Thanks.

>
> src/hotspot/share/oops/accessDecorators.hpp
> -------------------------------------------
> Looks like the decorator numbers got a bit mixed up:
>
> […]
> Stefan just told me you planned to fix this in a different patch?

Yes.  There’s still one more change to go that affects the numbering,
the removal of IN_CONCURRENT_ROOT.  I should have mentioned
that in the RFR email though.

> src/hotspot/share/oops/access.hpp
> ---------------------------------
> Could we turn this:
>
> 123                                              IS_ARRAY | IS_NOT_NULL |
> 124                                              IN_HEAP;
>
> Into a single line, with IN_HEAP first, like:
>
> 123                                              IN_HEAP | IS_ARRAY | IS_NOT_NULL;

Will do.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8205459: Rename Access API flag decorators

Per Liden
On 2018-06-21 18:26, Kim Barrett wrote:
>> On Jun 21, 2018, at 5:32 AM, Per Liden <[hidden email]> wrote:
>>
>> Looks good, just two comments:
>
> Thanks.

Btw, I don't need to see a new webrev.

/Per

>
>>
>> src/hotspot/share/oops/accessDecorators.hpp
>> -------------------------------------------
>> Looks like the decorator numbers got a bit mixed up:
>>
>> […]
>> Stefan just told me you planned to fix this in a different patch?
>
> Yes.  There’s still one more change to go that affects the numbering,
> the removal of IN_CONCURRENT_ROOT.  I should have mentioned
> that in the RFR email though.
>
>> src/hotspot/share/oops/access.hpp
>> ---------------------------------
>> Could we turn this:
>>
>> 123                                              IS_ARRAY | IS_NOT_NULL |
>> 124                                              IN_HEAP;
>>
>> Into a single line, with IN_HEAP first, like:
>>
>> 123                                              IN_HEAP | IS_ARRAY | IS_NOT_NULL;
>
> Will do.
>