RFR: cleanup - removed unneeded casts in jdi

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: cleanup - removed unneeded casts in jdi

Egor Ushakov
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/

I do not have rights to create JDK bug report directly, please create it
if it is needed for the procedure.

Thanks!

--
Egor Ushakov
Software Developer
JetBrains
http://www.jetbrains.com
The Drive to Develop

Reply | Threaded
Open this post in threaded view
|

Re: RFR: cleanup - removed unneeded casts in jdi

David Holmes
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. LocationImpl has decided how to
compare two LocaltionImp's (but doesn't even check they are in the same
VirtualMachine!). 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.

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.

David
-----

> I do not have rights to create JDK bug report directly, please create it
> if it is needed for the procedure.
>
> Thanks!
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: cleanup - removed unneeded casts in jdi

serguei.spitsyn@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!


Reply | Threaded
Open this post in threaded view
|

Re: RFR: cleanup - removed unneeded casts in jdi

David Holmes
Hi Serguei,

On 23/12/2017 6:04 PM, [hidden email] 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.

>> 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.

>> 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.

> 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.

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!
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: cleanup - removed unneeded casts in jdi

serguei.spitsyn@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, [hidden email] 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!
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: cleanup - removed unneeded casts in jdi

Egor Ushakov

Thanks for your comments!

I'll try to provide more details:
We have our own Location implementaion in IDEA code: GeneratedLocation.java
which is not intended to be used inside the jdi, but mostly to mock Location in our own APIs like PositionManager.java
Unfortunately some implementations keep the provided Location objects (both LocationImpl and ours) in collections (maybe sorted) so we have to prevent cast exceptions from compareTo somehow.
Hope it helps.

Egor

On 24-Dec-17 03:32, [hidden email] wrote:
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, [hidden email] 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!




-- 
Egor Ushakov
Software Developer
JetBrains
http://www.jetbrains.com
The Drive to Develop
Reply | Threaded
Open this post in threaded view
|

RE: RFR: cleanup - removed unneeded casts in jdi

Langer, Christoph

Hi Egor, David and Serguei,

 

I had a look at this, too. I would think this really calls out for a “public default int compareTo(Location other) {…}” in Location.java which uses the implementation out of LocationImpl.java. That way, all the suggested improvements for MirrorImpl.java can be done as well. And other implementers of Location, such as IntelliJ’s GeneratedLocation.java, would still build and won’t be necessarily wrong but could probably gradually remove their compareTo methods.

 

As for checking for the same VM within Location comparison, e.g. by using the equals() method, I guess this can be added. At least it should not add a notable cost. But I suggest to do it with a separate change, in case it turns out to be not a good idea and one needs to revert it.

 

@Egor: Would you mind to create an updated Webrev with an interface default method?

 

Best regards

Christoph

 

 

From: serviceability-dev [mailto:[hidden email]] On Behalf Of Egor Ushakov
Sent: Montag, 25. Dezember 2017 12:30
To: [hidden email]; David Holmes <[hidden email]>; [hidden email]
Subject: Re: RFR: cleanup - removed unneeded casts in jdi

 

Thanks for your comments!

I'll try to provide more details:
We have our own Location implementaion in IDEA code: GeneratedLocation.java
which is not intended to be used inside the jdi, but mostly to mock Location in our own APIs like PositionManager.java
Unfortunately some implementations keep the provided Location objects (both LocationImpl and ours) in collections (maybe sorted) so we have to prevent cast exceptions from compareTo somehow.
Hope it helps.

Egor

On 24-Dec-17 03:32, [hidden email] wrote:

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, [hidden email] 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!

 

 



-- 
Egor Ushakov
Software Developer
JetBrains
http://www.jetbrains.com
The Drive to Develop
Reply | Threaded
Open this post in threaded view
|

Re: RFR: cleanup - removed unneeded casts in jdi

David Holmes
Hi Christoph,

On 2/01/2018 4:41 PM, Langer, Christoph wrote:
> Hi Egor, David and Serguei,
>
> I had a look at this, too. I would think this really calls out for a
> “public default int compareTo(Location other) {…}” in Location.java

I think this could run into the "overloads instead of overrides" problem
that Brian describes here:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-November/050060.html

... unsure. But this would need a CSR request any way so hopefully any
issues with doing this would be caught there.

I'm very wary of adding default methods, though this may be such a
little used interface that it's not really an issue.

Cheers,
David

> which uses the implementation out of LocationImpl.java. That way, all
> the suggested improvements for MirrorImpl.java can be done as well. And
> other implementers of Location, such as IntelliJ’s
> GeneratedLocation.java, would still build and won’t be necessarily wrong
> but could probably gradually remove their compareTo methods.
>
> As for checking for the same VM within Location comparison, e.g. by
> using the equals() method, I guess this can be added. At least it should
> not add a notable cost. But I suggest to do it with a separate change,
> in case it turns out to be not a good idea and one needs to revert it.
>
> @Egor: Would you mind to create an updated Webrev with an interface
> default method?
>
> Best regards
>
> Christoph
>
> *From:*serviceability-dev
> [mailto:[hidden email]] *On Behalf Of *Egor
> Ushakov
> *Sent:* Montag, 25. Dezember 2017 12:30
> *To:* [hidden email]; David Holmes
> <[hidden email]>; [hidden email]
> *Subject:* Re: RFR: cleanup - removed unneeded casts in jdi
>
> Thanks for your comments!
>
> I'll try to provide more details:
> We have our own Location implementaion in IDEA code:
> GeneratedLocation.java
> <https://github.com/JetBrains/intellij-community/blob/29cdd102746d2252ef282082e7671128228489f8/java/debugger/impl/src/com/intellij/debugger/jdi/GeneratedLocation.java>
> which is not intended to be used inside the jdi, but mostly to mock
> Location in our own APIs like PositionManager.java
> <https://github.com/JetBrains/intellij-community/blob/306d705e1829bd3c74afc2489bfb7ed59d686b84/java/debugger/openapi/src/com/intellij/debugger/PositionManager.java>
> Unfortunately some implementations keep the provided Location objects
> (both LocationImpl and ours) in collections (maybe sorted) so we have to
> prevent cast exceptions from compareTo somehow.
> Hope it helps.
>
> Egor
>
> On 24-Dec-17 03:32, [hidden email]
> <mailto:[hidden email]> wrote:
>
>     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, [hidden email]
>         <mailto:[hidden email]> 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!
>
>
>
> --
>
> Egor Ushakov
>
> Software Developer
>
> JetBrains
>
> http://www.jetbrains.com
>
> The Drive to Develop
>
Reply | Threaded
Open this post in threaded view
|

RE: RFR: cleanup - removed unneeded casts in jdi

Langer, Christoph
Hi David,

thanks for pointing this out. I see what you mean.

However, you were the one who brought up the point that rather the Location interface should specify the means to compare two Locations :) And that would be an interface default method - or would there be another way? I guess, as there are no generics involved, the overloading instead of overriding thing should at least be more obvious for other implementers of the Location interface. But, for sure, I'm leaving the decision whether the default interface is the right thing here or not to better language and jdi experts than I am.  Egor's original proposal should work well, too, and is definitely less obtrusive.

BTW: your suggested change in MirrorImpl to go from "protected VirtualMachineImpl vm;" to "protected VirtualMachine vm;" would not really work out as VirtualMachineImpl extends MirrorImpl and in there VirtualMachineImpl is definitely needed. It's really a bit weird...

Best regards
Christoph

-----Original Message-----
From: David Holmes [mailto:[hidden email]]
Sent: Dienstag, 2. Januar 2018 08:31
To: Langer, Christoph <[hidden email]>; Egor Ushakov <[hidden email]>; [hidden email]; [hidden email]
Subject: Re: RFR: cleanup - removed unneeded casts in jdi

Hi Christoph,

On 2/01/2018 4:41 PM, Langer, Christoph wrote:
> Hi Egor, David and Serguei,
>
> I had a look at this, too. I would think this really calls out for a
> “public default int compareTo(Location other) {…}” in Location.java

I think this could run into the "overloads instead of overrides" problem
that Brian describes here:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-November/050060.html

... unsure. But this would need a CSR request any way so hopefully any
issues with doing this would be caught there.

I'm very wary of adding default methods, though this may be such a
little used interface that it's not really an issue.

Cheers,
David

> which uses the implementation out of LocationImpl.java. That way, all
> the suggested improvements for MirrorImpl.java can be done as well. And
> other implementers of Location, such as IntelliJ’s
> GeneratedLocation.java, would still build and won’t be necessarily wrong
> but could probably gradually remove their compareTo methods.
>
> As for checking for the same VM within Location comparison, e.g. by
> using the equals() method, I guess this can be added. At least it should
> not add a notable cost. But I suggest to do it with a separate change,
> in case it turns out to be not a good idea and one needs to revert it.
>
> @Egor: Would you mind to create an updated Webrev with an interface
> default method?
>
> Best regards
>
> Christoph
>
> *From:*serviceability-dev
> [mailto:[hidden email]] *On Behalf Of *Egor
> Ushakov
> *Sent:* Montag, 25. Dezember 2017 12:30
> *To:* [hidden email]; David Holmes
> <[hidden email]>; [hidden email]
> *Subject:* Re: RFR: cleanup - removed unneeded casts in jdi
>
> Thanks for your comments!
>
> I'll try to provide more details:
> We have our own Location implementaion in IDEA code:
> GeneratedLocation.java
> <https://github.com/JetBrains/intellij-community/blob/29cdd102746d2252ef282082e7671128228489f8/java/debugger/impl/src/com/intellij/debugger/jdi/GeneratedLocation.java>
> which is not intended to be used inside the jdi, but mostly to mock
> Location in our own APIs like PositionManager.java
> <https://github.com/JetBrains/intellij-community/blob/306d705e1829bd3c74afc2489bfb7ed59d686b84/java/debugger/openapi/src/com/intellij/debugger/PositionManager.java>
> Unfortunately some implementations keep the provided Location objects
> (both LocationImpl and ours) in collections (maybe sorted) so we have to
> prevent cast exceptions from compareTo somehow.
> Hope it helps.
>
> Egor
>
> On 24-Dec-17 03:32, [hidden email]
> <mailto:[hidden email]> wrote:
>
>     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, [hidden email]
>         <mailto:[hidden email]> 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!
>
>
>
> --
>
> Egor Ushakov
>
> Software Developer
>
> JetBrains
>
> http://www.jetbrains.com
>
> The Drive to Develop
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: cleanup - removed unneeded casts in jdi

David Holmes
On 2/01/2018 6:21 PM, Langer, Christoph wrote:
> Hi David,
>
> thanks for pointing this out. I see what you mean.
>
> However, you were the one who brought up the point that rather the Location interface should specify the means to compare two Locations :)

All I meant by that is that Location should _specify_ what it means to
compare two Locations. Any interface (or class for that matter) that
implements Comparable should provide an overriding specification for
compareTo.

> And that would be an interface default method - or would there be another way? I guess, as there are no generics involved, the overloading instead of overriding thing should at least be more obvious for other implementers of the Location interface. But, for sure, I'm leaving the decision whether the default interface is the right thing here or not to better language and jdi experts than I am.  Egor's original proposal should work well, too, and is definitely less obtrusive.

Yeah I'm going to punt on this one too. :)

> BTW: your suggested change in MirrorImpl to go from "protected VirtualMachineImpl vm;" to "protected VirtualMachine vm;" would not really work out as VirtualMachineImpl extends MirrorImpl and in there VirtualMachineImpl is definitely needed. It's really a bit weird...

Thanks for checking. Despite the use of interfaces and classes this
stuff doesn't really seem to be that amenable to supporting alternative
implementations of the interfaces.

Cheers,
David

> Best regards
> Christoph
>
> -----Original Message-----
> From: David Holmes [mailto:[hidden email]]
> Sent: Dienstag, 2. Januar 2018 08:31
> To: Langer, Christoph <[hidden email]>; Egor Ushakov <[hidden email]>; [hidden email]; [hidden email]
> Subject: Re: RFR: cleanup - removed unneeded casts in jdi
>
> Hi Christoph,
>
> On 2/01/2018 4:41 PM, Langer, Christoph wrote:
>> Hi Egor, David and Serguei,
>>
>> I had a look at this, too. I would think this really calls out for a
>> “public default int compareTo(Location other) {…}” in Location.java
>
> I think this could run into the "overloads instead of overrides" problem
> that Brian describes here:
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-November/050060.html
>
> ... unsure. But this would need a CSR request any way so hopefully any
> issues with doing this would be caught there.
>
> I'm very wary of adding default methods, though this may be such a
> little used interface that it's not really an issue.
>
> Cheers,
> David
>
>> which uses the implementation out of LocationImpl.java. That way, all
>> the suggested improvements for MirrorImpl.java can be done as well. And
>> other implementers of Location, such as IntelliJ’s
>> GeneratedLocation.java, would still build and won’t be necessarily wrong
>> but could probably gradually remove their compareTo methods.
>>
>> As for checking for the same VM within Location comparison, e.g. by
>> using the equals() method, I guess this can be added. At least it should
>> not add a notable cost. But I suggest to do it with a separate change,
>> in case it turns out to be not a good idea and one needs to revert it.
>>
>> @Egor: Would you mind to create an updated Webrev with an interface
>> default method?
>>
>> Best regards
>>
>> Christoph
>>
>> *From:*serviceability-dev
>> [mailto:[hidden email]] *On Behalf Of *Egor
>> Ushakov
>> *Sent:* Montag, 25. Dezember 2017 12:30
>> *To:* [hidden email]; David Holmes
>> <[hidden email]>; [hidden email]
>> *Subject:* Re: RFR: cleanup - removed unneeded casts in jdi
>>
>> Thanks for your comments!
>>
>> I'll try to provide more details:
>> We have our own Location implementaion in IDEA code:
>> GeneratedLocation.java
>> <https://github.com/JetBrains/intellij-community/blob/29cdd102746d2252ef282082e7671128228489f8/java/debugger/impl/src/com/intellij/debugger/jdi/GeneratedLocation.java>
>> which is not intended to be used inside the jdi, but mostly to mock
>> Location in our own APIs like PositionManager.java
>> <https://github.com/JetBrains/intellij-community/blob/306d705e1829bd3c74afc2489bfb7ed59d686b84/java/debugger/openapi/src/com/intellij/debugger/PositionManager.java>
>> Unfortunately some implementations keep the provided Location objects
>> (both LocationImpl and ours) in collections (maybe sorted) so we have to
>> prevent cast exceptions from compareTo somehow.
>> Hope it helps.
>>
>> Egor
>>
>> On 24-Dec-17 03:32, [hidden email]
>> <mailto:[hidden email]> wrote:
>>
>>      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, [hidden email]
>>          <mailto:[hidden email]> 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!
>>
>>
>>
>> --
>>
>> Egor Ushakov
>>
>> Software Developer
>>
>> JetBrains
>>
>> http://www.jetbrains.com
>>
>> The Drive to Develop
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: cleanup - removed unneeded casts in jdi

Alan Bateman
In reply to this post by David Holmes
On 22/12/2017 21:06, David Holmes wrote:

> :
>
> 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. LocationImpl has decided how to
> compare two LocaltionImp's (but doesn't even check they are in the
> same VirtualMachine!). 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.
I'm also wondering about the change to validateMirrors and this
assertion in the Mirror API:

"Any method on a Mirror that takes a Mirror as an parameter directly or
indirectly (e.g., as a element in a List) will throw VMMismatchException
if the mirrors are from different virtual machines."

There is an assumption in the implementation that two Mirror objects to
the same VM will be the same implementation type.

-Alan

Reply | Threaded
Open this post in threaded view
|

Re: RFR: cleanup - removed unneeded casts in jdi

Egor Ushakov
In reply to this post by David Holmes
Guys, thank you all for your comments! I'm a little bit lost now, how do
we proceed?


On 02-Jan-18 12:17, David Holmes wrote:

> On 2/01/2018 6:21 PM, Langer, Christoph wrote:
>> Hi David,
>>
>> thanks for pointing this out. I see what you mean.
>>
>> However, you were the one who brought up the point that rather the
>> Location interface should specify the means to compare two Locations :)
>
> All I meant by that is that Location should _specify_ what it means to
> compare two Locations. Any interface (or class for that matter) that
> implements Comparable should provide an overriding specification for
> compareTo.
>
>> And that would be an interface default method - or would there be
>> another way? I guess, as there are no generics involved, the
>> overloading instead of overriding thing should at least be more
>> obvious for other implementers of the Location interface. But, for
>> sure, I'm leaving the decision whether the default interface is the
>> right thing here or not to better language and jdi experts than I
>> am.  Egor's original proposal should work well, too, and is
>> definitely less obtrusive.
>
> Yeah I'm going to punt on this one too. :)
>
>> BTW: your suggested change in MirrorImpl to go from "protected
>> VirtualMachineImpl vm;" to "protected VirtualMachine vm;" would not
>> really work out as VirtualMachineImpl extends MirrorImpl and in there
>> VirtualMachineImpl is definitely needed. It's really a bit weird...
>
> Thanks for checking. Despite the use of interfaces and classes this
> stuff doesn't really seem to be that amenable to supporting
> alternative implementations of the interfaces.
>
> Cheers,
> David
>
>> Best regards
>> Christoph
>>
>> -----Original Message-----
>> From: David Holmes [mailto:[hidden email]]
>> Sent: Dienstag, 2. Januar 2018 08:31
>> To: Langer, Christoph <[hidden email]>; Egor Ushakov
>> <[hidden email]>; [hidden email];
>> [hidden email]
>> Subject: Re: RFR: cleanup - removed unneeded casts in jdi
>>
>> Hi Christoph,
>>
>> On 2/01/2018 4:41 PM, Langer, Christoph wrote:
>>> Hi Egor, David and Serguei,
>>>
>>> I had a look at this, too. I would think this really calls out for a
>>> “public default int compareTo(Location other) {…}” in Location.java
>>
>> I think this could run into the "overloads instead of overrides" problem
>> that Brian describes here:
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-November/050060.html 
>>
>>
>> ... unsure. But this would need a CSR request any way so hopefully any
>> issues with doing this would be caught there.
>>
>> I'm very wary of adding default methods, though this may be such a
>> little used interface that it's not really an issue.
>>
>> Cheers,
>> David
>>
>>> which uses the implementation out of LocationImpl.java. That way, all
>>> the suggested improvements for MirrorImpl.java can be done as well. And
>>> other implementers of Location, such as IntelliJ’s
>>> GeneratedLocation.java, would still build and won’t be necessarily
>>> wrong
>>> but could probably gradually remove their compareTo methods.
>>>
>>> As for checking for the same VM within Location comparison, e.g. by
>>> using the equals() method, I guess this can be added. At least it
>>> should
>>> not add a notable cost. But I suggest to do it with a separate change,
>>> in case it turns out to be not a good idea and one needs to revert it.
>>>
>>> @Egor: Would you mind to create an updated Webrev with an interface
>>> default method?
>>>
>>> Best regards
>>>
>>> Christoph
>>>
>>> *From:*serviceability-dev
>>> [mailto:[hidden email]] *On Behalf Of
>>> *Egor
>>> Ushakov
>>> *Sent:* Montag, 25. Dezember 2017 12:30
>>> *To:* [hidden email]; David Holmes
>>> <[hidden email]>; [hidden email]
>>> *Subject:* Re: RFR: cleanup - removed unneeded casts in jdi
>>>
>>> Thanks for your comments!
>>>
>>> I'll try to provide more details:
>>> We have our own Location implementaion in IDEA code:
>>> GeneratedLocation.java
>>> <https://github.com/JetBrains/intellij-community/blob/29cdd102746d2252ef282082e7671128228489f8/java/debugger/impl/src/com/intellij/debugger/jdi/GeneratedLocation.java>
>>>
>>> which is not intended to be used inside the jdi, but mostly to mock
>>> Location in our own APIs like PositionManager.java
>>> <https://github.com/JetBrains/intellij-community/blob/306d705e1829bd3c74afc2489bfb7ed59d686b84/java/debugger/openapi/src/com/intellij/debugger/PositionManager.java>
>>>
>>> Unfortunately some implementations keep the provided Location objects
>>> (both LocationImpl and ours) in collections (maybe sorted) so we
>>> have to
>>> prevent cast exceptions from compareTo somehow.
>>> Hope it helps.
>>>
>>> Egor
>>>
>>> On 24-Dec-17 03:32, [hidden email]
>>> <mailto:[hidden email]> wrote:
>>>
>>>      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, [hidden email]
>>>          <mailto:[hidden email]> 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!
>>>
>>>
>>>
>>> --
>>>
>>> Egor Ushakov
>>>
>>> Software Developer
>>>
>>> JetBrains
>>>
>>> http://www.jetbrains.com
>>>
>>> The Drive to Develop
>>>

--
Egor Ushakov
Software Developer
JetBrains
http://www.jetbrains.com
The Drive to Develop

Reply | Threaded
Open this post in threaded view
|

Re: RFR: cleanup - removed unneeded casts in jdi

David Holmes
Hi Egor,

On 15/01/2018 7:07 PM, Egor Ushakov wrote:
> Guys, thank you all for your comments! I'm a little bit lost now, how do
> we proceed?

I think your proposed changes can be taken as is. They have highlighted
some deficiencies in the existing API and implementation, but we don't
need to try and solve that problem now (or even at all).

You'll need a sponsor from the serviceability team (Hi Serguei!)

Thanks,
David

>
>
> On 02-Jan-18 12:17, David Holmes wrote:
>> On 2/01/2018 6:21 PM, Langer, Christoph wrote:
>>> Hi David,
>>>
>>> thanks for pointing this out. I see what you mean.
>>>
>>> However, you were the one who brought up the point that rather the
>>> Location interface should specify the means to compare two Locations :)
>>
>> All I meant by that is that Location should _specify_ what it means to
>> compare two Locations. Any interface (or class for that matter) that
>> implements Comparable should provide an overriding specification for
>> compareTo.
>>
>>> And that would be an interface default method - or would there be
>>> another way? I guess, as there are no generics involved, the
>>> overloading instead of overriding thing should at least be more
>>> obvious for other implementers of the Location interface. But, for
>>> sure, I'm leaving the decision whether the default interface is the
>>> right thing here or not to better language and jdi experts than I
>>> am.  Egor's original proposal should work well, too, and is
>>> definitely less obtrusive.
>>
>> Yeah I'm going to punt on this one too. :)
>>
>>> BTW: your suggested change in MirrorImpl to go from "protected
>>> VirtualMachineImpl vm;" to "protected VirtualMachine vm;" would not
>>> really work out as VirtualMachineImpl extends MirrorImpl and in there
>>> VirtualMachineImpl is definitely needed. It's really a bit weird...
>>
>> Thanks for checking. Despite the use of interfaces and classes this
>> stuff doesn't really seem to be that amenable to supporting
>> alternative implementations of the interfaces.
>>
>> Cheers,
>> David
>>
>>> Best regards
>>> Christoph
>>>
>>> -----Original Message-----
>>> From: David Holmes [mailto:[hidden email]]
>>> Sent: Dienstag, 2. Januar 2018 08:31
>>> To: Langer, Christoph <[hidden email]>; Egor Ushakov
>>> <[hidden email]>; [hidden email];
>>> [hidden email]
>>> Subject: Re: RFR: cleanup - removed unneeded casts in jdi
>>>
>>> Hi Christoph,
>>>
>>> On 2/01/2018 4:41 PM, Langer, Christoph wrote:
>>>> Hi Egor, David and Serguei,
>>>>
>>>> I had a look at this, too. I would think this really calls out for a
>>>> “public default int compareTo(Location other) {…}” in Location.java
>>>
>>> I think this could run into the "overloads instead of overrides" problem
>>> that Brian describes here:
>>>
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-November/050060.html 
>>>
>>>
>>> ... unsure. But this would need a CSR request any way so hopefully any
>>> issues with doing this would be caught there.
>>>
>>> I'm very wary of adding default methods, though this may be such a
>>> little used interface that it's not really an issue.
>>>
>>> Cheers,
>>> David
>>>
>>>> which uses the implementation out of LocationImpl.java. That way, all
>>>> the suggested improvements for MirrorImpl.java can be done as well. And
>>>> other implementers of Location, such as IntelliJ’s
>>>> GeneratedLocation.java, would still build and won’t be necessarily
>>>> wrong
>>>> but could probably gradually remove their compareTo methods.
>>>>
>>>> As for checking for the same VM within Location comparison, e.g. by
>>>> using the equals() method, I guess this can be added. At least it
>>>> should
>>>> not add a notable cost. But I suggest to do it with a separate change,
>>>> in case it turns out to be not a good idea and one needs to revert it.
>>>>
>>>> @Egor: Would you mind to create an updated Webrev with an interface
>>>> default method?
>>>>
>>>> Best regards
>>>>
>>>> Christoph
>>>>
>>>> *From:*serviceability-dev
>>>> [mailto:[hidden email]] *On Behalf Of
>>>> *Egor
>>>> Ushakov
>>>> *Sent:* Montag, 25. Dezember 2017 12:30
>>>> *To:* [hidden email]; David Holmes
>>>> <[hidden email]>; [hidden email]
>>>> *Subject:* Re: RFR: cleanup - removed unneeded casts in jdi
>>>>
>>>> Thanks for your comments!
>>>>
>>>> I'll try to provide more details:
>>>> We have our own Location implementaion in IDEA code:
>>>> GeneratedLocation.java
>>>> <https://github.com/JetBrains/intellij-community/blob/29cdd102746d2252ef282082e7671128228489f8/java/debugger/impl/src/com/intellij/debugger/jdi/GeneratedLocation.java>
>>>>
>>>> which is not intended to be used inside the jdi, but mostly to mock
>>>> Location in our own APIs like PositionManager.java
>>>> <https://github.com/JetBrains/intellij-community/blob/306d705e1829bd3c74afc2489bfb7ed59d686b84/java/debugger/openapi/src/com/intellij/debugger/PositionManager.java>
>>>>
>>>> Unfortunately some implementations keep the provided Location objects
>>>> (both LocationImpl and ours) in collections (maybe sorted) so we
>>>> have to
>>>> prevent cast exceptions from compareTo somehow.
>>>> Hope it helps.
>>>>
>>>> Egor
>>>>
>>>> On 24-Dec-17 03:32, [hidden email]
>>>> <mailto:[hidden email]> wrote:
>>>>
>>>>      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, [hidden email]
>>>>          <mailto:[hidden email]> 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!
>>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Egor Ushakov
>>>>
>>>> Software Developer
>>>>
>>>> JetBrains
>>>>
>>>> http://www.jetbrains.com
>>>>
>>>> The Drive to Develop
>>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: cleanup - removed unneeded casts in jdi

serguei.spitsyn@oracle.com
Hi Egor,

Just for a correct email record, could you, please, resend this RFR
with a correct subject line that includes the bug number and synopsis:
   8194143: remove unneeded casts in LocationImpl and MirrorImpl classes

Additionally, please, attach a committed patch for this fix,
so I could push this fix into the jdk/hs.


On 1/15/18 19:47, David Holmes wrote:

> Hi Egor,
>
> On 15/01/2018 7:07 PM, Egor Ushakov wrote:
>> Guys, thank you all for your comments! I'm a little bit lost now, how
>> do we proceed?
>
> I think your proposed changes can be taken as is. They have
> highlighted some deficiencies in the existing API and implementation,
> but we don't need to try and solve that problem now (or even at all).
>
> You'll need a sponsor from the serviceability team (Hi Serguei!)

Sure, I'll sponsor it, David!

Thanks,
Serguei

>
> Thanks,
> David
>
>>
>>
>> On 02-Jan-18 12:17, David Holmes wrote:
>>> On 2/01/2018 6:21 PM, Langer, Christoph wrote:
>>>> Hi David,
>>>>
>>>> thanks for pointing this out. I see what you mean.
>>>>
>>>> However, you were the one who brought up the point that rather the
>>>> Location interface should specify the means to compare two
>>>> Locations :)
>>>
>>> All I meant by that is that Location should _specify_ what it means
>>> to compare two Locations. Any interface (or class for that matter)
>>> that implements Comparable should provide an overriding
>>> specification for compareTo.
>>>
>>>> And that would be an interface default method - or would there be
>>>> another way? I guess, as there are no generics involved, the
>>>> overloading instead of overriding thing should at least be more
>>>> obvious for other implementers of the Location interface. But, for
>>>> sure, I'm leaving the decision whether the default interface is the
>>>> right thing here or not to better language and jdi experts than I
>>>> am.  Egor's original proposal should work well, too, and is
>>>> definitely less obtrusive.
>>>
>>> Yeah I'm going to punt on this one too. :)
>>>
>>>> BTW: your suggested change in MirrorImpl to go from "protected
>>>> VirtualMachineImpl vm;" to "protected VirtualMachine vm;" would not
>>>> really work out as VirtualMachineImpl extends MirrorImpl and in
>>>> there VirtualMachineImpl is definitely needed. It's really a bit
>>>> weird...
>>>
>>> Thanks for checking. Despite the use of interfaces and classes this
>>> stuff doesn't really seem to be that amenable to supporting
>>> alternative implementations of the interfaces.
>>>
>>> Cheers,
>>> David
>>>
>>>> Best regards
>>>> Christoph
>>>>
>>>> -----Original Message-----
>>>> From: David Holmes [mailto:[hidden email]]
>>>> Sent: Dienstag, 2. Januar 2018 08:31
>>>> To: Langer, Christoph <[hidden email]>; Egor Ushakov
>>>> <[hidden email]>; [hidden email];
>>>> [hidden email]
>>>> Subject: Re: RFR: cleanup - removed unneeded casts in jdi
>>>>
>>>> Hi Christoph,
>>>>
>>>> On 2/01/2018 4:41 PM, Langer, Christoph wrote:
>>>>> Hi Egor, David and Serguei,
>>>>>
>>>>> I had a look at this, too. I would think this really calls out for a
>>>>> “public default int compareTo(Location other) {…}” in Location.java
>>>>
>>>> I think this could run into the "overloads instead of overrides"
>>>> problem
>>>> that Brian describes here:
>>>>
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-November/050060.html 
>>>>
>>>>
>>>> ... unsure. But this would need a CSR request any way so hopefully any
>>>> issues with doing this would be caught there.
>>>>
>>>> I'm very wary of adding default methods, though this may be such a
>>>> little used interface that it's not really an issue.
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>> which uses the implementation out of LocationImpl.java. That way, all
>>>>> the suggested improvements for MirrorImpl.java can be done as
>>>>> well. And
>>>>> other implementers of Location, such as IntelliJ’s
>>>>> GeneratedLocation.java, would still build and won’t be necessarily
>>>>> wrong
>>>>> but could probably gradually remove their compareTo methods.
>>>>>
>>>>> As for checking for the same VM within Location comparison, e.g. by
>>>>> using the equals() method, I guess this can be added. At least it
>>>>> should
>>>>> not add a notable cost. But I suggest to do it with a separate
>>>>> change,
>>>>> in case it turns out to be not a good idea and one needs to revert
>>>>> it.
>>>>>
>>>>> @Egor: Would you mind to create an updated Webrev with an interface
>>>>> default method?
>>>>>
>>>>> Best regards
>>>>>
>>>>> Christoph
>>>>>
>>>>> *From:*serviceability-dev
>>>>> [mailto:[hidden email]] *On Behalf Of
>>>>> *Egor
>>>>> Ushakov
>>>>> *Sent:* Montag, 25. Dezember 2017 12:30
>>>>> *To:* [hidden email]; David Holmes
>>>>> <[hidden email]>; [hidden email]
>>>>> *Subject:* Re: RFR: cleanup - removed unneeded casts in jdi
>>>>>
>>>>> Thanks for your comments!
>>>>>
>>>>> I'll try to provide more details:
>>>>> We have our own Location implementaion in IDEA code:
>>>>> GeneratedLocation.java
>>>>> <https://github.com/JetBrains/intellij-community/blob/29cdd102746d2252ef282082e7671128228489f8/java/debugger/impl/src/com/intellij/debugger/jdi/GeneratedLocation.java>
>>>>>
>>>>> which is not intended to be used inside the jdi, but mostly to mock
>>>>> Location in our own APIs like PositionManager.java
>>>>> <https://github.com/JetBrains/intellij-community/blob/306d705e1829bd3c74afc2489bfb7ed59d686b84/java/debugger/openapi/src/com/intellij/debugger/PositionManager.java>
>>>>>
>>>>> Unfortunately some implementations keep the provided Location objects
>>>>> (both LocationImpl and ours) in collections (maybe sorted) so we
>>>>> have to
>>>>> prevent cast exceptions from compareTo somehow.
>>>>> Hope it helps.
>>>>>
>>>>> Egor
>>>>>
>>>>> On 24-Dec-17 03:32, [hidden email]
>>>>> <mailto:[hidden email]> wrote:
>>>>>
>>>>>      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, [hidden email]
>>>>>          <mailto:[hidden email]> 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!
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>>
>>>>> Egor Ushakov
>>>>>
>>>>> Software Developer
>>>>>
>>>>> JetBrains
>>>>>
>>>>> http://www.jetbrains.com
>>>>>
>>>>> The Drive to Develop
>>>>>
>>