This is the mail archive of the
libstdc++@gcc.gnu.org
mailing list for the libstdc++ project.
Re: vector lightweight debug mode
- From: FranÃois Dumont <frs dot dumont at gmail dot com>
- To: Jonathan Wakely <jwakely at redhat dot com>
- Cc: "libstdc++ at gcc dot gnu dot org" <libstdc++ at gcc dot gnu dot org>, gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Sat, 19 Sep 2015 09:31:01 +0200
- Subject: Re: vector lightweight debug mode
- Authentication-results: sourceware.org; auth=none
- References: <55F71189 dot 8080006 at gmail dot com> <20150914195038 dot GQ2631 at redhat dot com> <55F9C4F6 dot 6030706 at gmail dot com> <20150916202953 dot GE2631 at redhat dot com>
On 16/09/2015 22:29, Jonathan Wakely wrote:
>
>>>
>>>> constexpr bool __move_storage =
>>>> _Alloc_traits::_S_propagate_on_move_assign()
>>>> || _Alloc_traits::_S_always_equal();
>>>> @@ -778,7 +779,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>>> */
>>>> reference
>>>> operator[](size_type __n) _GLIBCXX_NOEXCEPT
>>>> - { return *(this->_M_impl._M_start + __n); }
>>>> + {
>>>> + __glibcxx_assert(__n < size());
>>>> + return *(this->_M_impl._M_start + __n);
>>>> + }
>>>
>>> This could use __glibcxx_requires_subscript(__n), see the attached
>>> patch.
>>
>> I thought you didn't want to use anything from debug.h so I try to
>> do with only __glibcxx_assert coming from c++config. I think your patch
>> is missing include of debug.h.
>>
>> But I was going to propose to use _Error_formatter also in this
>> mode, I do not see any reason to not do so. The attached patch does just
>> that.
>
> That pulls in extra dependencies on I/O and fprintf and things, which
> can cause code size to increase. Is it really worth it?
Not that much dependencies. We only need formatters.h in this mode which
has the following common includes:
#include <bits/c++config.h>
#include <bits/cpp_type_traits.h>
and if rtti is enabled the less common:
#include <typeinfo>
We would just leverage on the good job done to diagnose problems.
>
>
>>>> @@ -1051,6 +1071,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>>>> iterator
>>>> insert(const_iterator __position, size_type __n, const
>>>> value_type& __x)
>>>> {
>>>> + __glibcxx_assert(__position >= cbegin() && __position <= cend());
>>>> difference_type __offset = __position - cbegin();
>>>> _M_fill_insert(begin() + __offset, __n, __x);
>>>> return begin() + __offset;
>>>
>>> This is undefined behaviour, so I'd rather not add this check (I know
>>> it's on the google branch, but it's still undefined behaviour).
>>
>> Why ? Because of the >= operator usage ? Is the attached patch better ?
>> < and == operators are well defined for a random access iterator, no ?
>
> No, because it is undefined to compare iterators that belong to
> different containers, or to compare pointers that point to different
> arrays.
>
(Written before Christopher reply:)
At least program will compile only if iterator is coming from a vector
of the same type. So behavior is undefined only if user pass an invalid
iterator which is exactly what this check tries to detect, isn't it
paradoxical ? If this undefined behavior results in the program abortion
this is what should happen anyway. If it doesn't abort then the program
will definitely not behaves as expected so this check doesn't make
anything worst, no ?
François