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:serviceability-dev-boun...@openjdk.java.net] *On Behalf Of
*Egor
Ushakov
*Sent:* Montag, 25. Dezember 2017 12:30
*To:* serguei.spit...@oracle.com; David Holmes
<david.hol...@oracle.com>; serviceability-dev@openjdk.java.net
*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, serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com> 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, serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com> wrote:
Hi Egor and David,
Egor,
The fix looks good in general.
I've filed bug:
https://bugs.openjdk.java.net/browse/JDK-8194143
remove unneeded casts in LocationImpl and
MirrorImpl
classes
On 12/22/17 13:06, David Holmes wrote:
Hi Egor,
On 23/12/2017 1:32 AM, Egor Ushakov wrote:
Hi all,
could you please review and sponsor this small
cleanup removing unneeded casts in jdi
LocationImpl
and MirrorImpl.
They were preventing creating custom Location
and
Mirror implementations used for tests and IDEA
debugger impl.
http://cr.openjdk.java.net/~avu/egor.ushakov/cast_fix/
src/jdk.jdi/share/classes/com/sun/tools/jdi/LocationImpl.java
! public int compareTo(Location object) {
- LocationImpl other = (LocationImpl)object;
The existing code is somewhat suspect as the
Location
interface implements Comparable but it does not
specify
what it means to compare two Locations! That's a
bug in
itself.
Not sure, if it is really needed as it is abstract.
We could say: An implementation of the Location is
expected
to specify it.
That makes it impossible to compare different
implementations of
the Location interface. The functionality has to be
specified by
the interface.
We probably never needed to compare them before.
But such comparison can be needed for an IDE that has a deal
with
different JDI implementations.
LocationImpl has decided how to compare two
LocaltionImp's (but doesn't even check they are
in the
same VirtualMachine!).
Nice catch!
Interesting...
Should comparing of locations from different mirrors
be a
no-op?
Not sure if it would be right to throw a
VMMismatchException
in such cases.
Not sure - without knowing why we need to compare
Locations it's
hard to say.
Ok.
Can we generalize that to accommodate other Location
implementations?Your change allows for this to
happen,
but it will only work as expected if the other
Location
implementations use the same comparison basis as
LocationImpl - which is unspecified.
It is not clear, what you mean here.
What are the other Location implementations?
The ones that Egor is implementing and the reason for
this bug
report.
It is not clear to me why do they need their own Location
implementation.
A JDI implementation normally has one base
implementation of
the Location.
What would be a need to have multiple?
Egor indicated it was for use in testing and the IDEA
debugger.
It's apparent they have their own implementation of
Location,
but these instances have to interact with the default
LocationImpl implementations - else this bug report would
not be
needed.
Will need to look at it more closely after NY.
I'm going to vacation in a couple of hours until the Jan 3-rd.
Will probably have a limited internet access there.
I wish you, guys, happy Xmas and New Year and nice Holidays!
Thanks,
Serguei
Cheers,
David
And different JDI implementations are not supposed to
interact with each other, are they?
src/jdk.jdi/share/classes/com/sun/tools/jdi/MirrorImpl.java
Change looks good. It would also seem that now this
change is made that this:
37 protected VirtualMachineImpl vm;
38
39 MirrorImpl(VirtualMachine aVm) {
40 super();
41
42 // Yes, its a bit of a hack. But by
doing
it this
43 // way, this is the only place we
have to
change
44 // typing to substitute a new impl.
45 vm = (VirtualMachineImpl)aVm;
might reduce to:
37 protected VirtualMachine vm;
38
39 MirrorImpl(VirtualMachine aVm) {
40 super();
41 vm = aVm;
if we no longer depend on it being
VirtualMachineImpl
... and neither do subclasses.
Good suggestion.
Thanks,
Serguei
David
-----
I do not have rights to create JDK bug report
directly, please create it if it is needed
for the
procedure.
Thanks!
--
Egor Ushakov
Software Developer
JetBrains
http://www.jetbrains.com
The Drive to Develop
