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