On Thu, 29 Jun 2023 17:29:50 GMT, Coleen Phillimore <cole...@openjdk.org> 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