On Wed, 4 Mar 2026 06:04:35 GMT, Axel Boldt-Christmas <[email protected]>
wrote:
>> src/hotspot/share/oops/instanceKlass.cpp line 2212:
>>
>>> 2210: }
>>> 2211:
>>> 2212: bool InstanceKlass::find_local_flat_field_from_offset(int offset,
>>> fieldDescriptor* fd) const {
>>
>> The name of this method is confusing. The legacy
>> `InstanceKlass::find_local_field_from_offset()` returns the information of
>> the field at a given offset, but the new
>> `InstanceKlass::find_local_flat_field_from_offset()` returns either the flat
>> field at a given offset or the flat field declared in the current class that
>> encloses the specified offset. This dual behavior would deserve a comment to
>> make it more explicit.
>> The name of the method could also be improved, to make it more distinct than
>> the legacy `find_local_field_from_offset()`. May be
>> `InstanceKlass::find_local_flat_field_containing_offset()` ?
>> Other than that, the method's implementation looks fine, and it's usage in
>> the recursive method ValuePayload::assert_is_flat_field() looks fine too.
>
> I was also unsure what to call this. I see the function as just:
>> returns [...] the flat field declared in the current class that encloses the
>> specified offset.
>
> And
>> returns the flat field at a given offset
>
> I just a special case of that.
>
> `find_local_flat_field_containing_offset` seems like a fine name.
>
> I think it probably would have been clearer if `find_local_field_from_offset`
> made it clear that it means find local field starting at offset, it seems
> valid to interpret that function as finding the field at that offset,
> conceptually one byte in to an int field is still that int field.
The field offset is used as a field ID in CI and JNI code, I was concerned
about a risk of using the wrong method and getting the enclosing field instead
of the field at the exact offset.
Thank you for the renaming.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2186#discussion_r2891540944
