[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
From:       Daniil Titov <daniil.x.titov () oracle ! com>
Date:       2020-05-19 16:32:27
Message-ID: E8FC86BB-32C5-43EA-86C3-2D60634DD8BC () oracle ! com
[Download RAW message or body]

Hi Chris and Serguei,

Thank you for reviewing this change.

Best regards,
Daniil

On 5/18/20, 12:41 PM, "Chris Plummer" <chris.plummer@oracle.com> wrote:

    Looks good.

    thanks,

    Chris

    On 5/14/20 1:33 PM, 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.spitsyn@oracle.com" <serguei.spitsyn@oracle.com>
    > Date: Monday, May 11, 2020 at 11:53 AM
    > To: Daniil Titov <daniil.x.titov@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
    >
    >
    >
    >
    >
    >


[prev in list] [next in list] [prev in thread] [next in thread]