This is the mail archive of the
mailing list for the GCC project.
Re: [patch] Make vector::at() assertion message more useful (try #2)
- From: Paolo Carlini <paolo dot carlini at oracle dot com>
- To: Paul Pluzhnikov <ppluzhnikov at google dot com>
- Cc: Daniel KrÃgler <daniel dot kruegler at gmail dot com>, gcc-patches List <gcc-patches at gcc dot gnu dot org>, libstdc++ <libstdc++ at gcc dot gnu dot org>
- Date: Thu, 19 Sep 2013 00:00:51 +0200
- Subject: Re: [patch] Make vector::at() assertion message more useful (try #2)
- Authentication-results: sourceware.org; auth=none
- References: <ye6qhae0qpf8 dot fsf at elbrus2 dot mtv dot corp dot google dot com> <CAGNvRgDfHf6pYmBYC66HHhj6RobhiaiAAVEKj1FDxSWGxbYFTg at mail dot gmail dot com> <CALoOobMf3u8R_DVW5b2eRy05fBpB5unhriLMYdUEkT_E3DwtaA at mail dot gmail dot com> <CAGNvRgD6mK=-n7QhK=-NtfXZHOpCniXKc-riWUNfqZeVa8xufQ at mail dot gmail dot com> <CALoOobO6XEsrSM6jJFCQ=n00iY8qM=eujs-=LixNZyuya5f-cA at mail dot gmail dot com> <5232E2AF dot 4000106 at oracle dot com> <CALoOobMejGJLVVCszP8Joa+4B21UbvK4chyFUnjiCNPP0GJF0A at mail dot gmail dot com>
> Il giorno 18/set/2013, alle ore 21:38, Paul Pluzhnikov <firstname.lastname@example.org> ha scritto:
>> On Fri, Sep 13, 2013 at 3:02 AM, Paolo Carlini <email@example.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
> Replaced with __throw_out_of_range_fmt.
>> - Please consistently use __builtin_alloca everywhere, alloca isn't a
>> standard function.
>> - I would rather call the file itself snprintf_lite.cc, in order not to fool
>> somebody that it actually implements the whole snprintf.
>> - 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.
>> - 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.
> I also expanded snprintf_lite to handle '%s', as that comes handy inside
> string _M_check.
> 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.