Re: [patch] Make vector::at() assertion message more useful (try #2)

[Paolo Carlini <paolo dot carlini at oracle dot com>]

Hi,

> Il giorno 18/set/2013, alle ore 21:38, Paul Pluzhnikov <ppluzhnikov@google.com> ha scritto:
> 
>> On Fri, Sep 13, 2013 at 3:02 AM, Paolo Carlini <paolo.carlini@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