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-
