Re: RFR (S) 8201224: Make string buffer size dynamic

serguei.spit...@oracle.com Tue, 21 Aug 2018 13:44:09 -0700

Hi Jc,

It looks pretty good to me.

One minor comment:

http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.03/test/hotspot/jtreg/vmTestbase/vm/mlvm/indy/func/jvmti/share/IndyRedefineClass.c.udiff.html

     locStr = locationToString(jvmti_env, method, location);
+
+    if (locStr == NULL) {
+        NSK_DISPLAY0("Error in Single step event: locationToString failed\n");
+        gIsErrorOccured = JNI_TRUE;
+    }
+
     NSK_DISPLAY1("Single step event: %s\n", locStr);
     free(locStr);


  I'd suggest to put the use of locStr to the else statement as it is done in stepBreakPopReturn.c:

     } else {
         NSK_DISPLAY1("Single step event: %s\n", locStr);
         free(locStr);
     }

How did you test it?
Di you run all vm/mlvm tests?

Thanks,
Serguei

On 8/21/18 10:20, JC Beyler wrote:

Hi Alex,

Thanks for the review, I fixed the white-spaces here:

http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.03/

I'm missing a second reviewer though.

Thanks in advance,

Jc

On Wed, Aug 15, 2018 at 3:58 PM Alex Menkov <alexey.men...@oracle.com> wrote:

Hi Jc,

Looks good to me. I'm okay with using "buffer=NULL, len=0" approach.
The only note - could you fix indent of the inserted lines for
consistency (they have 2 spaces indents, but the files use 4 spaces
indents). No need to resend webrev.

--alex

On 08/14/2018 21:26, JC Beyler wrote:
> Hi Alex,
>
> Thanks for looking at it. That simplifies the webrev to this then:
> http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.02/
> <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.02/>
>
> I inlined a few comments on your answers.
>
> Thanks for the review and hopefully the new one is better :-)
>
> Thanks,
> Jc
>
>
> On Tue, Aug 14, 2018 at 3:16 PM Alex Menkov <alexey.men...@oracle.com
> <mailto:alexey.men...@oracle.com>> wrote:
>
> Hi Jc,
>
> Calculation of the required buffer size ("old fashion way") looks
> error-prone.
> I don't think we need to care about ancient SUSv2, I suppose C99 is
> supported by all (or almost all) compilers.
>
> But if you want to keep the code SUSv2-compatible, you can do
> something like
> char dummy = 0;
> int requiredSize = snprintf(&dummy, sizeof(dummy),
> formatString,...);
>
>
> Perfect :) For some reason I misread that snprintf would return the
> number of bytes written and thus would be at max the length.
>
> In this case, you cannot do it this way, the compiler produces warnings
> due to the dummy character can't even take the format. To solve it, I
> would have to create a buffer at least big enough for empty strings and
> small numbers. We can go down that route if you prefer to instead of
> using NULL.
>
>
> Other note:
> "if (obtained_size == len) " is useless
> obtained_size is a result of snprintf(result, len, ...)
> and snprintf returns number of chars _excluding_ terminating null, so
> obtained_size is always less than len.
>
>
> Technically this was to guard the case that we were not going over the
> limit as I misread the return of the method. You are right, sorry about
> that.
>
> Thanks again!
> Jc
>
>
> --alex
>
> On 08/10/2018 07:55, JC Beyler wrote:
> > Hi all,
> >
> > Anybody motivated to look at this change? :)
> >
> > Webrev: http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.01/
> <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/>
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/>
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8201224
> >
> > Let me know what you think!
> > Jc
> >
> >
> > On Wed, Aug 8, 2018 at 11:34 AM JC Beyler <jcbey...@google.com
> <mailto:jcbey...@google.com>
> > <mailto:jcbey...@google.com <mailto:jcbey...@google.com>>> wrote:
> >
> > Hi all,
> >
> > Here is a revised webrev that only does the stated fix in the
> bug. I
> > filed https://bugs.openjdk.java.net/browse/JDK-8209153 for fixing
> > the style.
> >
> > Webrev:
> http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.01/
> <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/>
> > <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.01/>
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8201224
> >
> > Let me know what you think!
> >
> > Thanks!
> > Jc
> >
> > On Fri, Aug 3, 2018 at 9:12 AM JC Beyler <jcbey...@google.com
> <mailto:jcbey...@google.com>
> > <mailto:jcbey...@google.com <mailto:jcbey...@google.com>>> wrote:
> >
> > Yes that was my train of thought as well (do it in various
> > webrevs and via various bugs for tracking). Is there a
> will or
> > interest to doing a few passes to just put these tests
> into the
> > same format once and for all? If so, I'll create a few
> bugs for
> > a few cleanup webrevs.
> >
> > On the other hand, often there is a worry that we lose
> history
> > due to the cleanup passes modifying each line of each file.
> >
> > Hence me asking before doing something like that :-)
> > Jc
> >
> > On Fri, Aug 3, 2018 at 7:45 AM Daniel D. Daugherty
> > <daniel.daughe...@oracle.com
> <mailto:daniel.daughe...@oracle.com>
> > <mailto:daniel.daughe...@oracle.com
> <mailto:daniel.daughe...@oracle.com>>> wrote:
> >
> > > Ps: A lot of these tests seem to be in GCC style
> (where
> > spaces are all
> > > over the place, 4 space indent, etc.), should we do a
> > one-pass per
> > > folder to make it into more standard hotspot style?
> >
> > If you do whitespace/style cleanups, please do them
> with a
> > separate bug.
> >
> > Dan
> >
> >
> >
> > On 8/3/18 12:11 AM, JC Beyler wrote:
> >> Hi all,
> >>
> >> Small change to the locationToString method to make the
> >> string dynamically generated.
> >>
> >> I added a bit of paranoia to handle the case where
> things
> >> might be changed so that an error would propagate to
> the test.
> >>
> >> Igor asked me to also at least fix the NSK_VERIFY
> tests in
> >> the file.
> >>
> >> Let me know what you think:
> >>
> >> Webrev:
> >> http://cr.openjdk.java.net/~jcbeyler/8201224/webrev.00/
> <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.00/>
> >>
> <http://cr.openjdk.java.net/%7Ejcbeyler/8201224/webrev.00/>
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8201224
> >>
> >> Thanks,
> >> Jc
> >>
> >> Ps: A lot of these tests seem to be in GCC style (where
> >> spaces are all over the place, 4 space indent, etc.),
> >> should we do a one-pass per folder to make it into more
> >> standard hotspot style?
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
>
>
>
> --
>
> Thanks,
> Jc

--

Thanks,
Jc

Re: RFR (S) 8201224: Make string buffer size dynamic

Reply via email to