Re: RFR: cleanup - removed unneeded casts in jdi

[serguei.spit...@oracle.com]

Hi Egor and David, Egor, The fix looks good in general. I've filed bug:   https://bugs.openjdk.java.net/browse/JDK-8194143     remove unneeded casts in LocationImpl and MirrorImpl classes On 12/22/17 13:06, David Holmes wrote: Hi Egor, On 23/12/2017 1:32 AM, Egor Ushakov wrote: Hi all, could you please review and sponsor this small cleanup removing unneeded casts in jdi LocationImpl and MirrorImpl. They were preventing creating custom Location and Mirror implementations used for tests and IDEA debugger impl. http://cr.openjdk.java.net/~avu/egor.ushakov/cast_fix/ src/jdk.jdi/share/classes/com/sun/tools/jdi/LocationImpl.java !     public int compareTo(Location object) { -        LocationImpl other = (LocationImpl)object; The existing code is somewhat suspect as the Location interface implements Comparable but it does not specify what it means to compare two Locations! That's a bug in itself. Not sure, if it is really needed as it is abstract. We could say: An implementation of the Location is expected to specify it. LocationImpl has decided how to compare two LocaltionImp's (but doesn't even check they are in the same VirtualMachine!). Nice catch! Interesting... Should comparing of locations from different mirrors be a no-op? Not sure if it would be right to throw a VMMismatchException in such cases. Can we generalize that to accommodate other Location implementations?Your change allows for this to happen, but it will only work as expected if the other Location implementations use the same comparison basis as LocationImpl - which is unspecified. It is not clear, what you mean here. What are the other Location implementations? A JDI implementation normally has one base implementation of the Location. What would be a need to have multiple? And different JDI implementations are not supposed to interact with each other, are they? src/jdk.jdi/share/classes/com/sun/tools/jdi/MirrorImpl.java Change looks good. It would also seem that now this change is made that this:   37     protected VirtualMachineImpl vm;   38   39     MirrorImpl(VirtualMachine aVm) {   40         super();   41   42         // Yes, its a bit of a hack. But by doing it this   43         // way, this is the only place we have to change   44         // typing to substitute a new impl.   45         vm = (VirtualMachineImpl)aVm; might reduce to:   37     protected VirtualMachine vm;   38   39     MirrorImpl(VirtualMachine aVm) {   40         super();   41         vm = aVm; if we no longer depend on it being VirtualMachineImpl ... and neither do subclasses. Good suggestion. Thanks, Serguei David ----- I do not have rights to create JDK bug report directly, please create it if it is needed for the procedure. Thanks!