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

[David Holmes]

Hi Magnus,

That all seems reasonable to me.

Thanks,
David
-----



On 16/04/2020 10:47 pm, 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