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