Hi,
> Il giorno 18/set/2013, alle ore 21:38, Paul Pluzhnikov
> <ppluzhni...@google.com> ha scritto:
>
>> On Fri, Sep 13, 2013 at 3:02 AM, Paolo Carlini <paolo.carl...@oracle.com>
>> wrote:
>>
>> - The game with the variadic and the non-variadic __throw_out_of_range makes
>> me a little nervous. Let's just name the new one differently, like
>> __throw_out_of_range_var.
>
> Replaced with __throw_out_of_range_fmt.
>
>> - Please consistently use __builtin_alloca everywhere, alloca isn't a
>> standard function.
>
> Done.
>
>> - I would rather call the file itself snprintf_lite.cc, in order not to fool
>> somebody that it actually implements the whole snprintf.
>
> Done.
>
>> - I'm a bit confused about __concat_size_t returning -1. Since it only
>> formats integers, I think we can be *sure* that the buffer is big enough.
>
> How so? The caller could do this:
>
> __snprintf_lite("aaa: %zu, 8, 12345678);
>
> By the time we get to __concat_size_t, we only have 3 characters left.
Ok, thanks, I think I misread the code. In general I meant that you can bound a
priori the number of chars you need to format an integer - that isn't the case
for floats, for example (the user may want a ridiculously large number of
decimal digits). But you are in fact already exploiting that for the value of
__ilen.
>
>> Then, if it returns -1 something is going *very badly* wrong, shouldn't we
>> __builtin_abort() or something similar?
>
> I've re-worked this part -- if this ever happens, throw a logic_error with
> a message to file a bug.
>
>> - In terms of buffer sizes, this comment:
>>
>> // enough for expanding up to 5 size_t's in the format.
>>
>> and then the actual code in __snprintf_lite makes me a little nervous.
>> Agreed, we are not going to overflow the buffer, but truncating with no
>> diagnostic whatsoever seems rather gross. We can probably sort out this
>> later, new ideas welcome, anyway.
>
> Re-worked.
>
>> - While we are at it, shouldn't we use the new facility at least in array,
>> vector<bool> and deque too? For consistency over the containers.
>
> Done.
>
> I also expanded snprintf_lite to handle '%s', as that comes handy inside
> string _M_check.
>
> Thanks,
>
> P.S. In the process of updating callers of __throw_out_of_range, I've
> discovered that two of them had already used __builtin_sprintf.
>
> P.P.S. Sorry this patch grew ... I can split it into parts if that's easier
> to review.
In general I like this version of the patch a lot. Thus assuming nobody else
has further comments over the next day or so, please commit it at your ease. In
the meanwhile, since you are also touching debug-mode and profile-mode, make
sure to run check-debug and check-profile too.
Thanks,
Paolo
