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