On Thu, 29 Jun 2023 17:29:50 GMT, Coleen Phillimore <[email protected]> wrote:
>> Please review change for mostly fixing return types in the constant pool and
>> metadata to fix -Wconversion warnings in JVMTI code. The order of
>> preference for changes are: 1. change the types to more distinct types
>> (fields in the constant pool are u2 because that's their size in the
>> classfile), 2. add direct int casts if the value has been checked in asserts
>> above, and 3. checked_cast<> if not verified, and 4. added some
>> pointer_delta_as_ints where needed.
>> Tested with tier1-4.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a comment about u1 cast to new_index for the ldc bytecode.
Looks good, thanks for this change! I listed a few considerations below, but if
you don't think they are necessary, what you have is just fine.
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 2195:
> 2193: case Bytecodes::_ldc:
> 2194: {
> 2195: u1 cp_index = *(bcp + 1);
Constant pool indices are usually u2, why does this need to be a u1?
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 2268:
> 2266: {
> 2267: address p = bcp + 1;
> 2268: int cp_index = Bytes::get_Java_u2(p);
Should this field also be a u2?
src/hotspot/share/prims/methodComparator.cpp line 79:
> 77: case Bytecodes::_instanceof : {
> 78: int cpi_old = s_old->get_index_u2();
> 79: int cpi_new = s_new->get_index_u2();
These constant pool accessors like `klass_at_noresolve` currently take in `int
which` but I think it's worth looking at if this is necessary. Constant pool
indices and constant pool cache indices seem to both be u2 so it might be a
better option to change the arguments to u2 here to avoid the need to cast.
-------------
Marked as reviewed by matsaave (Committer).
PR Review: https://git.openjdk.org/jdk/pull/14710#pullrequestreview-1505776921
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1246928389
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1246927933
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1246934498
