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
