RFR: cleanup - removed unneeded casts in jdi
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Jan 16 23:02:15 UTC 2018
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 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
>>>>>
>>
More information about the serviceability-dev
mailing list