On Tue, 3 Jun 2025 00:05:35 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> src/hotspot/share/utilities/packedTable.hpp line 38:
>>
>>> 36: uint32_t _key_mask;
>>> 37: unsigned int _value_shift;
>>> 38: uint32_t _value_mask;
>>
>> Aren't all 4 of these types the same? can you make them all uint32_t or all
>> unsigned int? (former preferred).
>
> Can you explain somewhere how fields are mapped to this? I assume they're
> sorted, for some reason I expected the packed table to be {name-cp-index,
> sig-cp-index, offset-in-fieldstream-for-direct-access}. Does every field get
> 4 ints ? So why is it packed into ```Array<u1>``` rather than just use
> ```Array<u4>```? So much packing code that I don't know how anyone could
> ever debug it.
Yes, in practice these all are of the same size, but in case of the masks (as
well as in case of arguments in API) I want to stress out that these are 32 bit
numbers. The `unsigned int`s are just 'some not too big number'.
Is there any general guidance on deciding between `unsigned int` (I suppose
just `unsigned` is not recommended), `uint32_t` and `u4`?
I was hoping that the comment on line 68 explains the intended use, but I can
be more verbose and document each method. When the packed table is used for
fieldinfo, it's { offset-in-fieldstream, index-in-fieldstream }. The Comparator
implementation can translate offset-in-fieldstream -> { name, signature } and
then do the comparison. The `index-in-fieldstream` is kind of second-class
citizen; we need to fill it into `FieldInfo` and it is not encoded in the
stream, therefore we need to encode it in the packed table.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24847#discussion_r2122780819
