RFR: 8247912: Make narrowOop a scoped enum

Kim Barrett kim.barrett at oracle.com
Tue Sep 22 13:14:58 UTC 2020


> On Sep 21, 2020, at 5:32 PM, Ioi Lam <iklam at openjdk.java.net> wrote:
> 
> On Mon, 21 Sep 2020 04:01:34 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
> 
>> Please review this change to the type narrowOop from a typedef for juint to a
>> scoped enum with uint32_t as the representation type.  This provides stronger
>> type checking when using this type.
>> […]
> 
> This looks good overall to me.
> 
> src/hotspot/share/oops/compressedOops.hpp line 142:
> 
>> 140:     // Shift by 32 is UB if size in bits of i is 32, e.g. on 32bit platform.
>> 141:     assert(((i >> 16) >> 16) == 0, "narrowOop overflow");
>> 142:     return static_cast<narrowOop>(static_cast<NarrowType>(i));
> 
> Should we have an assert that further restricts the range according to max heap size?

I’m updating this, in part in light of your question below.

> Also, should we allow only unsigned types for T? I think using signed types here is prone to error and should be
> flagged.

It turns out we can’t restrict it to unsigned types, at least not very easily.  For example,
CDS calls this with an intptr_t value.  I suppose CDS could do conversion itself before
calling this, but that seems a little odd.  And I don’t know how many more there are after
changing that one.  

> Changes requested by iklam (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/273




More information about the shenandoah-dev mailing list