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