[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: cleanup - removed unneeded casts in jdi
From: Egor Ushakov <egor.ushakov () jetbrains ! com>
Date: 2017-12-25 11:29:55
Message-ID: 6f30371a-fbbd-54c7-714d-aa98e637a8e3 () jetbrains ! com
[Download RAW message or body]
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/29cdd102746d2252ef282082e7671128 \
228489f8/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.spitsyn@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.spitsyn@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
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Thanks for your comments! <br>
</p>
I'll try to provide more details:<br>
We have our own Location implementaion in IDEA code: <a
moz-do-not-send="true"
href="https://github.com/JetBrains/intellij-community/blob/29cdd102746d2252ef282082e76 \
71128228489f8/java/debugger/impl/src/com/intellij/debugger/jdi/GeneratedLocation.java">GeneratedLocation.java</a><br>
which is not intended to be used inside the jdi, but mostly to mock
Location in our own APIs like <a moz-do-not-send="true"
href="https://github.com/JetBrains/intellij-community/blob/306d705e1829bd3c74afc2489bf \
b7ed59d686b84/java/debugger/openapi/src/com/intellij/debugger/PositionManager.java">PositionManager.java</a><br>
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.<br>
Hope it helps.<br>
<br>
Egor<br>
<br>
<div class="moz-cite-prefix">On 24-Dec-17 03:32,
<a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br> \
</div> <blockquote type="cite"
cite="mid:24d79366-218e-6c4b-99a3-132469df3844@oracle.com">Hi
David,
<br>
<br>
Thank you for the explanations!
<br>
I've got your points.
<br>
<br>
<br>
On 12/23/17 15:32, David Holmes wrote:
<br>
<blockquote type="cite">Hi Serguei,
<br>
<br>
On 23/12/2017 6:04 PM, <a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote: <br>
<blockquote type="cite">Hi Egor and David,
<br>
<br>
<br>
Egor,
<br>
<br>
The fix looks good in general.
<br>
I've filed bug:
<br>
<a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8194143">https://bugs.openjdk.java.net/browse/JDK-8194143</a>
<br>
remove unneeded casts in LocationImpl and MirrorImpl
classes
<br>
<br>
<br>
On 12/22/17 13:06, David Holmes wrote:
<br>
<blockquote type="cite">Hi Egor,
<br>
<br>
On 23/12/2017 1:32 AM, Egor Ushakov wrote:
<br>
<blockquote type="cite">Hi all,
<br>
<br>
could you please review and sponsor this small cleanup
removing unneeded casts in jdi LocationImpl and
MirrorImpl.
<br>
They were preventing creating custom Location and Mirror
implementations used for tests and IDEA debugger impl.
<br>
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~avu/egor.ushakov/cast_fix/">http://cr.openjdk.java.net/~avu/egor.ushakov/cast_fix/</a>
<br>
</blockquote>
<br>
src/jdk.jdi/share/classes/com/sun/tools/jdi/LocationImpl.java
<br>
<br>
! public int compareTo(Location object) {
<br>
- LocationImpl other = (LocationImpl)object;
<br>
<br>
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.
<br>
</blockquote>
<br>
Not sure, if it is really needed as it is abstract.
<br>
We could say: An implementation of the Location is expected to
specify it.
<br>
</blockquote>
<br>
That makes it impossible to compare different implementations of
the Location interface. The functionality has to be specified by
the interface.
<br>
</blockquote>
<br>
We probably never needed to compare them before.
<br>
But such comparison can be needed for an IDE that has a deal with
different JDI implementations.
<br>
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">LocationImpl has decided how to
compare two LocaltionImp's (but doesn't even check they are
in the same VirtualMachine!).
<br>
</blockquote>
<br>
Nice catch!
<br>
Interesting...
<br>
Should comparing of locations from different mirrors be a
no-op?
<br>
Not sure if it would be right to throw a VMMismatchException
in such cases.
<br>
</blockquote>
<br>
Not sure - without knowing why we need to compare Locations it's
hard to say.
<br>
</blockquote>
<br>
Ok.
<br>
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">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.
<br>
</blockquote>
<br>
It is not clear, what you mean here.
<br>
What are the other Location implementations?
<br>
</blockquote>
<br>
The ones that Egor is implementing and the reason for this bug
report.
<br>
</blockquote>
<br>
It is not clear to me why do they need their own Location
implementation.
<br>
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">A JDI implementation normally has one
base implementation of the Location.
<br>
What would be a need to have multiple?
<br>
</blockquote>
<br>
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.
<br>
</blockquote>
<br>
Will need to look at it more closely after NY.
<br>
I'm going to vacation in a couple of hours until the Jan 3-rd.
<br>
Will probably have a limited internet access there.
<br>
<br>
I wish you, guys, happy Xmas and New Year and nice Holidays!
<br>
<br>
Thanks,
<br>
Serguei
<br>
<br>
<br>
<blockquote type="cite">
<br>
Cheers,
<br>
David
<br>
<br>
<blockquote type="cite">And different JDI implementations are
not supposed to interact with each other, are they?
<br>
<br>
<br>
<blockquote \
type="cite">src/jdk.jdi/share/classes/com/sun/tools/jdi/MirrorImpl.java <br>
<br>
Change looks good. It would also seem that now this change
is made that this:
<br>
<br>
37 protected VirtualMachineImpl vm;
<br>
38
<br>
39 MirrorImpl(VirtualMachine aVm) {
<br>
40 super();
<br>
41
<br>
42 // Yes, its a bit of a hack. But by doing it
this
<br>
43 // way, this is the only place we have to
change
<br>
44 // typing to substitute a new impl.
<br>
45 vm = (VirtualMachineImpl)aVm;
<br>
<br>
might reduce to:
<br>
<br>
37 protected VirtualMachine vm;
<br>
38
<br>
39 MirrorImpl(VirtualMachine aVm) {
<br>
40 super();
<br>
41 vm = aVm;
<br>
<br>
if we no longer depend on it being VirtualMachineImpl ...
and neither do subclasses.
<br>
</blockquote>
<br>
Good suggestion.
<br>
<br>
<br>
Thanks,
<br>
Serguei
<br>
<br>
<blockquote type="cite">
<br>
David
<br>
-----
<br>
<br>
<blockquote type="cite">I do not have rights to create JDK
bug report directly, please create it if it is needed for
the procedure.
<br>
<br>
Thanks!
<br>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
<pre class="moz-signature" cols="72">--
Egor Ushakov
Software Developer
JetBrains
<a class="moz-txt-link-freetext" \
href="http://www.jetbrains.com">http://www.jetbrains.com</a> The Drive to \
Develop</pre> </body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic