RFR: cleanup - removed unneeded casts in jdi
David Holmes
david.holmes at oracle.com
Tue Jan 2 07:31:06 UTC 2018
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 at openjdk.java.net] *On Behalf Of *Egor
> Ushakov
> *Sent:* Montag, 25. Dezember 2017 12:30
> *To:* serguei.spitsyn at oracle.com; David Holmes
> <david.holmes at oracle.com>; serviceability-dev at 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.spitsyn at oracle.com
> <mailto:serguei.spitsyn at 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 at oracle.com
> <mailto:serguei.spitsyn at 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
>
More information about the serviceability-dev
mailing list