RFR: 8256365: Clean up vtable initialization code

[Erik Österlund]

On Mon, 16 Nov 2020 20:38:22 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> I was looking through this code because of JDK-8061949 and want to do some minor cleanups.
> 1. There's a function in the wrong place (is_override)
> 2. methodHandles that use mh()->is_native(), with extra (),
> 3. some methods declared with TRAPS, that don't trap
> 4. some multi-clause conditionals with confusing formatting
> 5. extra InstanceKlass::cast() casts
> 6. some useless asserts
> 7. and potentially a bug with RedefineClasses where the method being added to the vtable may have been redefined in the constraint verification call. (noreg-hard)
> 
> Tested with tier1-3.

I don't think I like having nasty and subtle bug fixes and subtle behavioural changes hidden in what otherwise looks like a large (ish) cleanup patch. Could we do the bug fix first, and then rebase this mostly cleanup related change on top of that? As is, I have to read every line very carefully to see if it is just a cleanup or a subtle behaviour change.

src/hotspot/share/oops/klassVtable.cpp line 230:

> 228:           HandleMark hm(THREAD);
> 229:           assert(default_methods->at(i)->is_method(), "must be a Method*");
> 230:           methodHandle mh(THREAD, default_methods->at(i));

Maybe wrap the method in a {} block so you can see when it goes out of scope and don't get tempted to use it after it becomes invalid.

src/hotspot/share/oops/klassVtable.cpp line 474:

> 472:     // (TBD: put in a method to throw NoSuchMethodError if this slot is ever used.)
> 473:     if (super_method->name() == name && super_method->signature() == signature &&
> 474:         (!klass->is_interface() ||

Is _klass and klass really the same? I thought they could be different. If so, this looks like a subtle behavioural change hidden in a large cleanup patch.

-------------

Changes requested by eosterlund (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1236