RFR (S) 8201224: Make string buffer size dynamic

Alex Menkov alexey.menkov at oracle.com
Wed Aug 15 22:58:32 UTC 2018


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


More information about the serviceability-dev mailing list