[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: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2018-01-16 23:02:15
Message-ID: 3a5b3c57-b2cd-c81c-125f-1f4982c244cc () oracle ! com
[Download RAW message or body]
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:david.holmes@oracle.com]
> > > > Sent: Dienstag, 2. Januar 2018 08:31
> > > > To: Langer, Christoph <christoph.langer@sap.com>; Egor Ushakov
> > > > <egor.ushakov@jetbrains.com>; serguei.spitsyn@oracle.com;
> > > > serviceability-dev@openjdk.java.net
> > > > 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:serviceability-dev-bounces@openjdk.java.net] *On Behalf Of
> > > > > *Egor
> > > > > Ushakov
> > > > > *Sent:* Montag, 25. Dezember 2017 12:30
> > > > > *To:* serguei.spitsyn@oracle.com; David Holmes
> > > > > <david.holmes@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/29cdd102746d2252ef2820 \
> > > > > 82e7671128228489f8/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/306d705e1829bd3c74afc2 \
> > > > > 489bfb7ed59d686b84/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
> > > > > <mailto: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
> > > > > <mailto: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
> > > > >
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic