Hi Magnus,
This looks pretty good to me.
I hope, Joe gave some insight for Comparable's.
Thanks,
Serguei
On 4/16/20 05:47, Magnus Ihse Bursie wrote:
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
