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

[Daniel D. Daugherty]

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...


>>
>>      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...


>
>>
>>
>> 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.

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-
>>
>