On Sat, 3 Sep 2022 23:46:41 GMT, John R Rose <jr...@openjdk.org> wrote:
>> Refactor code from inside of CompressedStream into its own unit.
>>
>> This code is likely to be used in future refactorings, such as JDK-8292818
>> (replace 96-bit representation for field metadata with variable-sized
>> streams).
>>
>> Add gtests.
>
> John R Rose has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add more support for SA including vmStructs; tweak some debug code
Thank you for adding the SA and debug.cpp for debugging this.
src/hotspot/share/code/compressedStream.cpp line 85:
> 83: const int min_expansion = UNSIGNED5::MAX_LENGTH;
> 84: if (nsize < min_expansion*2)
> 85: nsize = min_expansion*2;
I think our coding style guide recommends having {} around even one line code
inside if and else statements. I usually ignore this if it's on the same line,
but on a separate line, it seems worth pointing out.
src/hotspot/share/utilities/unsigned5.cpp line 35:
> 33: template u4 UNSIGNED5::read_uint(u_char* array, int& offset_rw, int
> limit, AGS);
> 34: template void UNSIGNED5::write_uint(uint32_t value, u_char* array, int&
> offset_rw, int limit, AGS);
> 35: template int UNSIGNED5::check_length(u_char* array, int offset, int
> limit, AGS);
I don't know why you'd need these explicit instantiations. The entire
templates are in the hpp files so the caller will instantiate them.
src/hotspot/share/utilities/unsigned5.hpp line 174:
> 172:
> 173: // returns the encoded byte length of an unsigned 32-bit int
> 174: static constexpr int encoded_length(uint32_t value) {
Should all of these functions be private?
src/hotspot/share/utilities/unsigned5.hpp line 312:
> 310: : _array(const_cast<ARR&>(array)), _limit_ptr(NULL)
> 311: // note: if _limit_ptr is NULL, the ARR& is never reassigned
> 312: { limit_init(); _position = 0; }
indent needed.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/Unsigned5.java
line 59:
> 57: hval = db.lookupIntConstant("UNSIGNED5::H").intValue();
> 58: lval = db.lookupIntConstant("UNSIGNED5::L").intValue();
> 59: xval = db.lookupIntConstant("UNSIGNED5::X").intValue();
These constants aren't likely to change ever, are they? I don't see why we'd
need the SA to get these constants from the VM. It could just hard-code them
like you have here. Then UNSIGNED5 doesn't have to make VMStructs a friend
class.
Also, there isn't a compatibility issue with older SA - it might work sometimes
but the SA version should match the JVM that is being debugged.
test/hotspot/gtest/utilities/test_unsigned5.cpp line 31:
> 29: // T.I.L.
> 30: // $ sh ./configure ... --with-gtest=<...>/googletest ...
> 31: // $ make exploded-test TEST=gtest:unsigned5
What does T.I.L mean? This isn't the only way to run this test, so don't
include this.
-------------
PR: https://git.openjdk.org/jdk/pull/10067
