RFR: cleanup - removed unneeded casts in jdi

[serguei.spitsyn at oracle.com]

Hi David,

Thank you for the explanations!
I've got your points.


On 12/23/17 15:32, David Holmes wrote:
> Hi Serguei,
>
> On 23/12/2017 6:04 PM, serguei.spitsyn at oracle.com wrote:
>> 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.
>
> That makes it impossible to compare different implementations of the 
> Location interface. The functionality has to be specified by the 
> interface.

We probably never needed to compare them before.
But such comparison can be needed for an IDE that has a deal with 
different JDI implementations.


>>> 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.
>
> Not sure - without knowing why we need to compare Locations it's hard 
> to say.

Ok.


>>> 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?
>
> The ones that Egor is implementing and the reason for this bug report.

It is not clear to me why do they need their own Location implementation.


>> A JDI implementation normally has one base implementation of the 
>> Location.
>> What would be a need to have multiple?
>
> Egor indicated it was for use in testing and the IDEA debugger. It's 
> apparent they have their own implementation of Location, but these 
> instances have to interact with the default LocationImpl 
> implementations - else this bug report would not be needed.

Will need to look at it more closely after NY.
I'm going to vacation in a couple of hours until the Jan 3-rd.
Will probably have a limited internet access there.

I wish you, guys, happy Xmas and New Year and nice Holidays!

Thanks,
Serguei


>
> Cheers,
> David
>
>> 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!
>>>>
>>