Very nice! Good to go. /Staffan
On 29 apr 2014, at 10:57, Jaroslav Bachorik <[email protected]> wrote: > On 29.4.2014 10:12, [email protected] wrote: >> Hi Jaroslav, >> >> make/data/jdwp/jdwp.spec >> Just a minor comment: an extra space or a TAB in the "staticmethod": >> >> 1150 (Error INVALID_METHODID "methodID is not the ID of a >> static method in " > > Gone ... > http://cr.openjdk.java.net/~jbachorik/8031195/webrev.07 > > Could I have a "seal-of-approval" from a Reviewer, please? > > -JB- > >> >> >> Reviewed. >> >> Thanks, >> Serguei >> >> >> On 4/29/14 12:57 AM, Jaroslav Bachorik wrote: >>> Hi Serguei, >>> >>> On 29.4.2014 01:02, [email protected] wrote: >>>> 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. >>> >>> Yeah, I had great support from you guys when undertaking this! >>> >>> I've corrected the wordings and fixed the typos. >>> >>> http://cr.openjdk.java.net/~jbachorik/8031195/webrev.06 >>> >>> I hope these will be the last changes necessary. >>> >>> -JB- >>> >>> >>>> >>>> >>>> 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- >>>>>> >>>>> >>>> >>>> >>> >> >
