On Thu, 29 Jun 2023 17:23:44 GMT, Matias Saavedra Silva <matsa...@openjdk.org>
wrote:
>> 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.
>
> 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?
This could be a u2 to avoid confusion. Since it's ldc, the cp_index in the ldc
bytecode is only a u1 but this didn't get a Wconversion error so I should
probably keep it as int.
> 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?
It could be a u2, but since find_new_index's parameter is int and didn't need a
cast further down, I didn't change it to one.
> 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.
I had to change these two lines because BytecodeStream::get_index_u2 returns an
int, so got the warning and this didn't need to be declared with u2.
get_index_u2() could be fixed to return a u2 but I didn't want to go that far
as no casts were involved in this change.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247101683
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247069649
PR Review Comment: https://git.openjdk.org/jdk/pull/14710#discussion_r1247053822
