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-
