RFR: JDK-8242943 Fix all remaining unchecked warnings in jdk.hotspot.agent

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Thu Apr 16 12:47:35 UTC 2020


This is the final part of removing all warnings from the build of 
jdk.hotspot.agent. This patch includes a number of non-trivial fixes for 
the few remaining unchecked warnings.

The good news is that with this fix (and after the recent removal of 
Nashorn and rmic), the JDK build is finally complete free from warnings 
for the first time ever! EPIC!!!!!1!!! :-)

The bad news is that this patch is probably the most complex of all I've 
posted so far. :-/

I'll break down the changes in four groups:

* ConstMethod superclass issue

ConstMethod current extends VMObject. However, it is treated as a 
Metadata (Metadata is a subtype of VMObject). For instance, this code 
appears in Metadata.java:
   metadataConstructor.addMapping("ConstMethod", ConstMethod.class);

I believe it is just an oversight that ConstMethod does not also extend 
Metadata like the rest of the classes added to the metadataConstructor 
mapping, one which has not been caught without the extra type safety 
given by generics.

* ProfileData casting

In several places, a ProfileData item is extracted from a collection, 
its type checked with instanceof, and then acted on accordingly. (Yeah, 
nice and clean OOP abstraction at work! :-))

This works well most of the time, but when casting to ReceiverTypeData 
or CallTypeDataInterface, the type include generic parametrisation, so 
it needs to be cast to e.g. ReceiverTypeData<ciKlass,ciMethod>. 
Unfortunately, this cannot be verified using instanceof, due to generic 
type erasure, and thus the compiler considers this an unchecked cast. 
(At least, this is my understanding of what is happening.) As far as I 
can tell, this is legitimate code, and the only way to really handle 
this (barring a more extensive rewrite) is to disable the warning for 
these cases.

* Generification of the InstanceConstructor hierarchy

I have properly generified the InstanceConstructor hierarchy, with the 
VirtualConstructor, StaticBaseConstructor and VirtualBaseConstructor 
subclasses. (And also the related helper class VMObjectFactory.)

Most of it turned out OK (and with improved type safety), but there was 
one piece of code that is a bit unfortunate. In VirtualBaseConstructor, 
we have the following:
             @SuppressWarnings("unchecked")
             Class<? extends T> lookedUpClass = (Class<? extends 
T>)Class.forName(packageName + "." + superType.getName());
             c = lookedUpClass;

That is, we send in a name for a class and assumes that it will resolve 
to a subtype of T, but there is no way to enforce this with type safety. 
I have verified all current callers to the constructor so that they all 
abide to this rule (of course, otherwise the code would not have worked, 
but I checked it anyway). The alternative to suppressing the warning is 
to redesign the entire API, so we do not send in a class name, but 
instead a Class<? extends T>, and use that to extract the name instead. 
At this point, I don't find it worth it. This is, after all, no worse 
than it was before.

* SortableTableModel and TableModelComparator

This one was quite messy. The tables are being used in quite different 
ways in different places, which made it hard to generify in a good way. 
A lot of it revolves around keeping the data as Objects and casting it 
to a known type. (Some of this behavior comes from the fact that they 
extend old Swing classes which does this.) I've tried to tighten it up 
as much as possible by introducing increased type safety by 
generification, but in the end, it was not fully possible to do without 
a major surgery of the entire class structure. As above, most of it 
turned out OK, but not all.

The tricky one here is the helper TableModelComparator. This one took me 
a while to wrap my head around. It implements Comparator, but the 
compare() method takes "rows" from the model, which are just expressed 
as Objects, and left to subclasses to define differently. For one of the 
subclasses it uses the type T that the TableModel is created around, but 
in other it uses an independent domain model. :-/ Anyway. The compare() 
method then extracts data for the individual columns of each row using 
the method getValueForColumn(), and compare them pairwise. This data 
from the rows are supposed to implement Comparable.

In the end, I think I got it pretty OK, but I'm still an apprentice when 
it comes to generics black magic. The subclasses of TableModelComparator 
want to return different objects from getValueForColumn() for different 
columns in the row, like Long or String. They are all Comparable, but 
String is Comparable<String> and Long is Comparable<Long>. So I needed 
to specify the abstract method as returning Comparable<?>, since 
Comparable<String> is not Comparable<Object>.

But then, for reasons that I do not fully fathom, I could not specify

Comparable<?> o1 = getValueForColumn(row1, columns[i]);
Comparable<?> o2 = getValueForColumn(row2, columns[i]);
int result = o1.compareTo(o2);

because the compiler did not accept the call to compareTo().

I did try to sacrifice a black rooster at midnight and walking backwards 
in a circle three time, to no avail. Maybe the problem was that it was 
not full moon or that I have no cat. In the end, I casted o1 and o2 to 
Comparable<Object> and suppressed the warning for that cast.

If anyone knows what rituals I need to perform to make this work, or -- 
even better -- how to fix the code properly, please let me know!

Bug: https://bugs.openjdk.java.net/browse/JDK-8242943
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8242943-SA-final-unchecked-warnings/webrev.01

/Magnus


More information about the serviceability-dev mailing list