RFR: cleanup - removed unneeded casts in jdi
Egor Ushakov
egor.ushakov at jetbrains.com
Mon Jan 15 09:07:37 UTC 2018
Guys, thank you all for your comments! I'm a little bit lost now, how do
we proceed?
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 at oracle.com]
>> Sent: Dienstag, 2. Januar 2018 08:31
>> To: Langer, Christoph <christoph.langer at sap.com>; Egor Ushakov
>> <egor.ushakov at jetbrains.com>; serguei.spitsyn at oracle.com;
>> serviceability-dev at 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 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
>>>
--
Egor Ushakov
Software Developer
JetBrains
http://www.jetbrains.com
The Drive to Develop
More information about the serviceability-dev
mailing list