Hi Daniil, Thank you for the update! It looks good to me.
> Regarding CR for the JDWP spec issues I guess, you wanted to say "CSR", not CR. :) Thanks, Serguei On 5/14/20 13:33, Daniil Titov wrote:
Hi Serguei and Chris, Please review a new version of the change [1] that addresses your comments. Testing: Mach5 tier1-tier5 tests successfully passed. Regarding CR for the JDWP spec issues related to missing type information in some of the protocol packet descriptions [3], as Chris has just noticed we really don't need it, so I withdrew this CR. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 [3] https://bugs.openjdk.java.net/browse/JDK-8245057 Thank you, Daniil From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> Date: Monday, May 11, 2020 at 11:53 AM To: Daniil Titov <daniil.x.ti...@oracle.com>, serviceability-dev <serviceability-dev@openjdk.java.net> Subject: Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools Hi Daniil, It looks pretty good in general. A couple of nits below. http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html + void *cursor; + jbyte argumentTag; + jint argIndex = 0; + jvalue *argument = request->arguments;; . . . void *cursor; jint argIndex = 0; + jbyte argumentTag; jvalue *argument = request->arguments; It is better if the local variables 'cursor' and 'argumentTag' get initialized. There is double semicolon: "arguments;;" http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html 43 static inline jbyte basicType(const char *signature) { It'd be nice to rename it to basicTypeTag() to get it unified with other functions below. It is more safe to run tier5 as well. Thanks, Serguei On 5/9/20 09:29, Daniil Titov wrote: Please review a change[1] that centralizes the signature processing in serviceability tools to make it capable of being easily extensible in the future. Testing: Mach5 tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 Thank you, Daniil
