RFR 8139163: InstanceKlass::cast passes through NULL (original) (raw)

Kim Barrett kim.barrett at oracle.com
Fri Oct 23 20:30:16 UTC 2015

On Oct 22, 2015, at 8:10 PM, Kim Barrett <kim.barrett at oracle.com> wrote:

On Oct 22, 2015, at 4:05 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:

Summary: Reduce raw (InstanceKlass*) casts and InstanceKlass::cast, which no long allows null Somewhat of a tedious thing to code review (sorry). Tested with internal remote-build-and-test. open webrev at http://cr.openjdk.java.net/~coleenp/8139163.01/ bug link https://bugs.openjdk.java.net/browse/JDK-8139163.01 Thanks, Coleen Only a partial review; I didn't get through all the files, and have run out of time today. Here's what I have so far. I'll continue looking tomorrow.

Here’s the rest. I’ll reply to your responses separately.

src/share/vm/oops/klass.cpp 563 if (oop_is_instance()) { 564 InstanceKlass* ik = InstanceKlass::cast(const_cast<Klass*>(this));

Instead of casting away const and using InstanceKlass::cast, how about just

const InstanceKlass* ik = static_cast<const InstanceKlass*>(this);

src/share/vm/oops/klass.cpp 691 bool Klass::verify_vtable_index(int i) { 692 if (oop_is_instance()) { 693 int limit = InstanceKlass::cast(this)->vtable_length()/vtableEntry::size(); 694 assert(i >= 0 && i < limit, "index %d out of bounds %d", i, limit); 695 } else { 696 assert(oop_is_array(), "Must be"); 697 int limit = ArrayKlass::cast(this)->vtable_length()/vtableEntry::size(); 698 assert(i >= 0 && i < limit, "index %d out of bounds %d", i, limit); 699 } 700 return true; 701 }

Why are there any casts here at all? vtable_length is a member of Klass. E.g. why not:

bool Klass::verify_vtable_index(int i) { int limit = vtable_length() / vtableEntry::size(); assert(i >= 0 && i < limit, "index %d out of bounds %d, i, limit); return true; }

src/share/vm/oops/klassVtable.cpp 67 vtable_length = super == NULL ? 0 : InstanceKlass::cast(super)->vtable_length();

I don't think we need a cast here; vtable_length is a member of Klass.

src/share/vm/oops/klassVtable.cpp 130 // copy methods from superKlass 131 // can't inherit from array class, so must be InstanceKlass 132 // InstanceKlass::cast asserts that the Klass is an InstanceKlass 133 InstanceKlass* sk = InstanceKlass::cast(super()); 134 klassVtable* superVtable = sk->vtable(); 135 assert(superVtable->length() <= _length, "vtable too short");

New comment line 132 isn't very interesting, even if the cast is retained.

But why bother casting at all? Just

klassVtable* superVtable = super->vtable();

should be sufficient. And we don't actually care about whether it's an ArrayKlass or InstanceKlass here. If we did, then just assert that the super is an InstanceKlass (as in the old code) would be better.

src/share/vm/oops/klass.hpp

Pre-existing: Why aren't Klass::vtable and Klass::vtable_length pure virtual?

src/share/vm/oops/klassVtable.cpp 761 Klass* cursuper; 762 // Iterate on all superclasses, which should have instanceKlasses 763 // Note that we explicitly look for overpasses at each level. 764 // Overpasses may or may not exist for supers for pass 1, 765 // they should have been created for pass 2 and later. 766 767 for (cursuper = super; cursuper != NULL; cursuper = cursuper->super()) 768 { 769 if (InstanceKlass::cast(cursuper)->find_local_method(name, signature,

I would put the declaration for cursuper in the for-loop initialization, e.g.

for (Klass* cursuper = super; ...) ...

Pre-existing: 762 // Iterate on all superclasses, which should have instanceKlasses

"should have" => "should be" ? Also need a period.

src/share/vm/oops/method.cpp 302 return method_holder()->name();

Thank you!

src/share/vm/services/threadService.cpp 888 obj->klass()->external_name()); 912 waitingToLockBlocker->klass()->external_name());

Pre-existing: Shouldn't there be a ResourceMark somewhere nearby associated with these calls to external_name?

More information about the hotspot-dev mailing list