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:[email protected]]
On Behalf Of Egor Ushakov
Sent: Montag, 25. Dezember 2017 12:30
To: [email protected]; David Holmes <[email protected]>;
[email protected]
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,
[email protected]<mailto:[email protected]> 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,
[email protected]<mailto:[email protected]> 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
