RFR (S) 8201224: Make string buffer size dynamic

JC Beyler jcbeyler at google.com
Tue Aug 21 17:20:48 UTC 2018


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.menkov at 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.menkov at oracle.com
> > <mailto:alexey.menkov at 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 <jcbeyler at google.com
> >     <mailto:jcbeyler at google.com>
> >      > <mailto:jcbeyler at google.com <mailto:jcbeyler at 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 <jcbeyler at google.com
> >     <mailto:jcbeyler at google.com>
> >      >     <mailto:jcbeyler at google.com <mailto:jcbeyler at 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.daugherty at oracle.com
> >     <mailto:daniel.daugherty at oracle.com>
> >      >         <mailto:daniel.daugherty at oracle.com
> >     <mailto:daniel.daugherty at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180821/cc760fab/attachment-0001.html>


More information about the serviceability-dev mailing list