RFR: 8241080: Consolidate signature parsing code in serviceability tools

[Daniil Titov]

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.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
Date: Monday, May 11, 2020 at 11:53 AM
To: Daniil Titov <daniil.x.titov at oracle.com>, serviceability-dev <serviceability-dev at 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