On Mon, 6 Jan 2025 16:46:25 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Coleen Phillimore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove unused SA function. > > src/hotspot/share/interpreter/linkResolver.cpp line 586: > >> 584: // We need to change "protected" to "public". >> 585: assert(flags.is_protected(), "clone not protected?"); >> 586: u2 new_flags = flags.as_unsigned_short(); > > Nit: Should this also be replaced with `as_method_flags()`? Thanks Serguei, I replaced this one and a couple of as_field_flags() so that as_unsigned_short() is more limited to the cases where we don't want masking. > src/hotspot/share/opto/memnode.cpp line 1985: > >> 1983: // The field is Klass::_access_flags. Return its (constant) value. >> 1984: // (Folds up the 2nd indirection in >> Reflection.getClassAccessFlags(aClassConstant).) >> 1985: assert(this->Opcode() == Op_LoadUS, "must load an unsigned short >> from _access_flags"); > > Nit: This can be unified with line 1979 and also get rid of `this->`. 1979 and 1985 are in different branches of an if statement (address of modifier flags vs access flags) so needs to be repeated. But I did remove the this-> > src/hotspot/share/prims/jvm.cpp line 2472: > >> 2470: u2 field_access_flags = >> InstanceKlass::cast(k)->field_access_flags(field_index); >> 2471: // This & should be unnecessary. >> 2472: assert((field_access_flags & JVM_RECOGNIZED_FIELD_MODIFIERS) == >> field_access_flags, "already masked"); > > Nit: Yes, it is better to remove the lines: 2471-2472. fixed. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1904829376 PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1904830369 PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1904829864
