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

[serguei.spitsyn at oracle.com]

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