RFR 8031195: Support default and static interface methods in JDI, JDWP and JDB
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Apr 28 23:02:31 UTC 2014
Hi Jaroslav,
I looked at the new webrev:
http://cr.openjdk.java.net/~jbachorik/8031195/webrev.05/
The fixes look fine to me modulo comments below.
This is a great job in general.
On 4/28/14 2:59 AM, Jaroslav Bachorik wrote:
> Thanks Dan!
Indeed, Dan, thank you for the thorough review and nice catches!
And Jaroslav, thank you for your patience!
>
>
> 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.
This has not been fixed yet, changing to the following would work Ok:
1282 "suspended by an event or by*a* command. "
>>
>> 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, something like this is needed:
. . .
1147 (ErrorSet
1148 (Error INVALID_CLASS "clazz is not the ID of a class.")
1149 (Error INVALID_OBJECT "clazz is not a known ID.")
1150 (Error INVALID_METHODID "methodID is not the ID of a*static* method* in"
"this class type or one of its superclasses*.")
. . .
1320 (ErrorSet
1321 (Error INVALID_CLASS "clazz is not the ID of an interface.")
1322 (Error INVALID_OBJECT "clazz is not a known ID.")
1323 (Error INVALID_METHODID "methodID is not the ID of a*static* method* in this"**
** ***"interface* type or is the ID of a static initializer*.")
. . .
1661 (ErrorSet
1662 (Error INVALID_OBJECT)
1663 (Error INVALID_CLASS "clazz is not the ID of a reference "
1664 "type.")
1665 (Error INVALID_METHODID "methodID is not the ID of a*n instance* method* in this object's type** or"**
** "**one of its superclasses, superinterfaces, or implemented interfaces*.")
>
>>
>> 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 ...)
This one is still unchanged:
184 * loaded through the enclosing*class's* class loader.
Also need to be fixed in the src/share/classes/com/sun/jdi/ClassType.java:
106 * enclosing class's class loader). Primitive values must be
233 * loaded through the enclosing class's class loader.
270 * enclosing class's class loader). Primitive arguments must be
338 * loaded through the enclosing class's class loader.
>
>>
>> line 189: * @throws VMCannotBeModifiedException ...
>> Please add the following after line 189:
>>
>> *
>> * @since 1.8
Important catch!
>>
>
> 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?
Probably, there is no need to list the above exceptions as they are
"unchecked exceptions".
At least, it is Ok to skip them in the "throws" list (IMHO).
>
>>
>>
>> src/share/classes/com/sun/jdi/Method.java
>> line 144: * false otherwise
>> Please add the following after line 144:
>>
>> *
>> * @since 1.8
Important catch!
>
> 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.
I looked at the refactoring very thoroughly.
The changes look fine.
>>
>> 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.
I looked at the refactoring very thoroughly.
The changes look fine.
>>
>> 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.
>
>>
>> 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.
>
>>
>> test/com/sun/jdi/InterfaceMethodsTest.java
>> Very nice test!
Indeed!
Thanks,
Serguei
>>
>> Dan
>
> The updated webrev -
> http://cr.openjdk.java.net/~jbachorik/8031195/webrev.05/
>
> -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-
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140428/91611093/attachment.html>
More information about the serviceability-dev
mailing list