RFR 8031195: Support default and static interface methods in JDI, JDWP and JDB

Tue Apr 29 07:32:20 UTC 2014

On 28.4.2014 19:45, Daniel D. Daugherty wrote:
> On 4/28/14 3:59 AM, Jaroslav Bachorik wrote:
>> Thanks Dan!
>>
>>
>> On 25.4.2014 20:08, Daniel D. Daugherty wrote:
>>> On 4/24/14 6:52 AM, Jaroslav Bachorik wrote:
>>>> Please, review the following patch:
>>>>
>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8031195
>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8031195/webrev.04
>>>
>>> make/data/jdwp/jdwp.spec
>>>      line 1256: "<p>Since JDWP version 1.8
>>>          Missing the ending double quote.
>>
>> Fixed.
>>
>>>
>>>      line 1281: "suspended by an event or by command. "
>>>          "by command" isn't clear. "by this command", "by a command"
>>>          or something else more specific would be good.
>
> You didn't say anything about this one...

The same as the one below. Copied over from the ClassType#InvokeMethod 
documentation. If it is to be changed it should be changed in both places.

>
>
>>>
>>>      line 1322: (Error INVALID_METHODID  "methodID is not the ID of a
>>> method.")
>>>          The above description permits a 'methodID' value that
>>>          is not for a method in the 'clazz' interface. Perhaps
>>>          add "in the interface specified by clazz" at the end
>>>          of the description?
>>
>> This text is copied over from the ClassType#InvokeMethod. Should I
>> change it there too?
>
> Probably. Check with Staffan L for another set of eyes...
>
>
>>
>>>
>>> src/share/back/VirtualMachineImpl.c
>>>      No comments.
>>>
>>> src/share/back/debugDispatch.c
>>>      No comments.
>>>
>>> src/share/back/util.c
>>>      No comments.
>>>
>>> src/share/classes/com/sun/jdi/ClassType.java
>>>      No comments.
>>>
>>> src/share/classes/com/sun/jdi/InterfaceType.java
>>>      line 88: * but not a static initializer.
>>>          The 'jdwp.spec' wording does not mention this restriction.
>>
>> Mentioned this restriction in jdwp.spec
>>
>>>
>>>      typo line 107: * enclosing class's class loader).
>>>      typo line 184: *         loaded through the enclosing class's class
>>> loader.
>>>                 ->  enclosing class' class loader
>>
>> Fixed. Also in ClassType (the comments were copied over from there
>> with typos ...)
>>
>>>
>>>      line 189: * @throws VMCannotBeModifiedException ...
>>>          Please add the following after line 189:
>>>
>>>              *
>>>              * @since 1.8
>>>
>>
>> Done.
>>
>>>      line 193-196: These exception are not named in the throws clause:
>>>          IllegalArgumentException, VMCannotBeModifiedException
>>
>> They are runtime exceptions. Should I list them in the throws clause
>> regardless of that?
>
> I should have checked for that. I'm pretty sure we don't put
> runtime exceptions in the 'throws' clause... Of course, I'm
> not enough of a Java programmer to know why putting them there
> would be a bad idea or bad style or...

It would be basically a no-op. Since they are unchecked exceptions the 
API users can freely choose to ignore them.

>
>
>>
>>>
>>>
>>> src/share/classes/com/sun/jdi/Method.java
>>>      line 144: * false otherwise
>>>          Please add the following after line 144:
>>>
>>>              *
>>>              * @since 1.8
>>
>> Done.
>>
>>>
>>> src/share/classes/com/sun/jdi/ObjectReference.java
>>>      No comments.
>>>
>>> src/share/classes/com/sun/tools/example/debug/expr/LValue.java
>>>      No comments.
>>>
>>> src/share/classes/com/sun/tools/jdi/ClassTypeImpl.java
>>>      line 32: final public class ClassTypeImpl extends InvokableTypeImpl
>>>          The switch to "final" caught my eye. I presume that
>>>          SA-JDI does not extend this implementation class.
>>
>> To my best knowledge it does not.
>>
>>>
>>>      Most of these changes appear to be due to refactoring with
>>>      the new InvokableTypeImpl.java. Tried to do a visual diff
>>>      between the common parts of this file and InvokableTypeImpl.java.
>>>      Didn't see anything obviously wrong.
>>>
>>> src/share/classes/com/sun/tools/jdi/InterfaceTypeImpl.java
>>>      Most of these changes appear to be due to refactoring with
>>>      the new InvokableTypeImpl.java.  Tried to do a visual diff
>>>      between the common parts of this file and InvokableTypeImpl.java.
>>>      Didn't see anything obviously wrong.
>>>
>>> src/share/classes/com/sun/tools/jdi/MethodImpl.java
>>>      No comments.
>>>
>>> src/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java
>>>      No comments.
>>>
>>> src/share/classes/com/sun/tools/jdi/VirtualMachineManagerImpl.java
>>>      No comments.
>>>
>>> src/share/back/InterfaceTypeImpl.c src/share/back/ClassTypeImpl.c
>>>      No comments.
>>>
>>> src/share/back/InterfaceTypeImpl.h src/share/back/ClassTypeImpl.h
>>>      No comments.
>>>
>>> src/share/classes/com/sun/tools/jdi/InvokableTypeImpl.java
>>>      Most of this code came from refactoring  ClassTypeImpl.java or
>>>      InterfaceTypeImpl.java.
>>>
>>>      line 98: throws clause does not mention:
>>>          IllegalArgumentException or VMCannotBeModifiedException
>>
>> This is a runtime exception. It hasn't been mentioned in the
>> ClassType#invokeMethod() throws clause too.
>
> Same as above.
>
>
>>
>>>
>>>          But I also have to wonder why this JavaDoc is here since
>>>          this is an impl class...
>>
>> Just to add expressiveness. This method is actually declared by the
>> both interfaces, ClassType and InterfaceType and it kind of made sense
>> to have the shared implementation documented. The cleaner solution
>> would probably be to factor out a shared superinterface for ClassType
>> and InterfaceType and declare invokeMethod() there. But that would be
>> more disruptive than playing just with the implementations.
>>
>>>
>>> test/com/sun/jdi/EvalInterfaceStatic.sh
>>>      line 35: #  the above error occurs.  jdb doesnt notice that this is
>>>          Not sure what "the above error" is. I don't see an error
>>>          example above this line.
>>>
>>>          typo: "doesnt" -> "doesn't"
>>
>> This code comment is completely wrong - a remnant of copy-paste :/ I
>> forgot to clean it up. Sorry.
>
> No problem.
>
>
>>
>>>
>>> test/com/sun/jdi/InterfaceMethodsTest.java
>>>      Very nice test!
>>>
>>> Dan
>>
>> The updated webrev -
>> http://cr.openjdk.java.net/~jbachorik/8031195/webrev.05/
>
> Sounds good. Don't know when I'll get a chance for a re-review so
> please don't wait for me.

Ok. Thanks for taking time to review this!

-JB-

>
> Dan
>
>
>>
>> -JB-
>>
>>>
>>>
>>>>
>>>> With JDK8 it became possible to have methods with implementation in
>>>> interfaces - static and default interface methods. However the JDI and
>>>> JDWP were not updated to reflect these capabilities so it is not
>>>> currently possible to invoke a static or default interface method
>>>> programatically from the debugger.
>>>>
>>>> This patch adds support for static and default interface methods to
>>>> JDI, JDWP and JDB.
>>>>
>>>> Thanks,
>>>>
>>>> -JB-
>>>
>>
>