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