On Tue, 6 Jan 2026 23:46:44 GMT, David Holmes <[email protected]> wrote:
>> Larry Cable has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> JDK-8327246: updated copyright year. fixed Capitialization nit and
>> restructured use of InstanceKlass local as per comments
>
> src/hotspot/share/oops/instanceKlass.cpp line 2386:
>
>> 2384: oop pd = java_lang_Class::protection_domain(k->java_mirror());
>> 2385:
>> 2386: if (pd != nullptr && (ik = pd->klass()->is_instance_klass() ?
>> InstanceKlass::cast(pd->klass()) : nullptr) != nullptr) {
>
> Is a non-instance-class possible here??
good question, but I think defensive coding is warranted, and this is not a
performance sensitive code path IMO,
> src/hotspot/share/oops/instanceKlass.cpp line 2397:
>
>> 2395: oop cs = pd->obj_field(csfd.offset());
>> 2396:
>> 2397: if (cs != nullptr && (ik = cs->klass()->is_instance_klass() ?
>> InstanceKlass::cast(cs->klass()) : nullptr) != nullptr) {
>
> Is a non-instance-class possible here??
good question, but I think defensive coding is warranted, and this is not a
performance sensitive code path IMO
> src/hotspot/share/oops/instanceKlass.cpp line 2406:
>
>> 2404: oop loc = cs->obj_field(locfd.offset());
>> 2405:
>> 2406: if (loc != nullptr && loc->klass() ==
>> vmClasses::String_klass()) {
>
> Why are you checking the class type?
I am simply reusing an existing pattern I found elsewhere in the corpus where
code that accessed a field by name with the intent to process as a String
tested the type, its my practice to code with safety, while its unlikely that
the type of the field will change, I think its safer to test prior to invoking
a type specific API.
If you feel strongly about this, I will remove the klass comparision. :)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29048#discussion_r2676996740
PR Review Comment: https://git.openjdk.org/jdk/pull/29048#discussion_r2677014140
PR Review Comment: https://git.openjdk.org/jdk/pull/29048#discussion_r2677011507
