On Tue, 3 Mar 2026 21:27:20 GMT, Frederic Parain <[email protected]> wrote:
>> Axel Boldt-Christmas has updated the pull request with a new target base due
>> to a merge or a rebase. The incremental webrev excludes the unrelated
>> changes brought in by the merge/rebase. The pull request contains three
>> additional commits since the last revision:
>>
>> - Rename `from_offset` to `containing_offset`
>> - Merge remote-tracking branch 'upstream_valhalla/lworld' into JDK-8378519
>> - 8378519: [lworld] Fold fieldDescriptor / ResolvedFieldEntry checks into
>> the FlatFieldPayload constructor
>
> 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.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2186#discussion_r2882007242
