gul-cpp / gul14

General Utility Library for C++14
https://gul14.info/
GNU Lesser General Public License v2.1
2 stars 1 forks source link

SmallVector: Fix compiler warning #23

Closed Finii closed 1 year ago

Finii commented 1 year ago

[why] When compiled with -O3 (aka Meson option optimization=3) GCC 12.2 optimizes too good:

In file included from /usr/include/c++/12/algorithm:60,
                 from ../tests/test_SmallVector.cc:23:
In static member function ‘static _Tp* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(const _Tp*, const _Tp*, _Tp*) [with _Tp = int; bool _IsMove = false]’,
    inlined from ‘_OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = const int*; _OI = int*]’ at /usr/include/c++/12/bits/stl_algobase.h:495:30,
    inlined from ‘_OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = false; _II = const int*; _OI = int*]’ at /usr/include/c++/12/bits/stl_algobase.h:522:42,
    inlined from ‘_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = const int*; _OI = int*]’ at /usr/include/c++/12/bits/stl_algobase.h:529:31,
    inlined from ‘_OI std::copy(_II, _II, _OI) [with _II = const int*; _OI = int*]’ at /usr/include/c++/12/bits/stl_algobase.h:620:7,
    inlined from ‘_OutputIterator std::__copy_n(_RandomAccessIterator, _Size, _OutputIterator, random_access_iterator_tag) [with _RandomAccessIterator = const int*; _Size = unsigned int; _OutputIterator = int*]’ at /usr/include/c++/12/bits/stl_algo.h:728:23,
    inlined from ‘_OIter std::copy_n(_IIter, _Size, _OIter) [with _IIter = const int*; _Size = unsigned int; _OIter = int*]’ at /usr/include/c++/12/bits/stl_algo.h:760:27,
    inlined from ‘gul14::SmallVector<ElementT, in_capacity>::ValueType* gul14::SmallVector<ElementT, in_capacity>::insert(ConstIterator, InputIterator, InputIterator) [with InputIterator = const int*; <template-parameter-2-2> = void; ElementT = int; long unsigned int in_capacity = 10]’ at ../include/gul14/SmallVector.h:851:20,
    inlined from ‘gul14::SmallVector<ElementT, in_capacity>::ValueType* gul14::SmallVector<ElementT, in_capacity>::insert(ConstIterator, std::initializer_list<_Tp>) [with ElementT = int; long unsigned int in_capacity = 10]’ at ../include/gul14/SmallVector.h:876:22,
    inlined from ‘void ____C_A_T_C_H____T_E_S_T____90()’ at ../tests/test_SmallVector.cc:1278:20:
/usr/include/c++/12/bits/stl_algobase.h:431:30: warning: argument 2 null where non-null expected [-Wnonnull]
  431 |             __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
      |             ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/12/bits/stl_algobase.h:431:30: note: in a call to built-in function ‘void* __builtin_memmove(void*, const void*, long unsigned int)’

[how] This is the case when we add zero elements from some iterator-like source. We pass that two iterators on unchecked and finally want to memmove the elements but there are none.

Now we just check if there is something to actually insert and bail out if not.

Fixes: #22

soerengrunewald commented 1 year ago

If num_elements < 1 the container is empty, right? And idx can not become negative? Then we could just return begin() without adding + idx, i guess. Since you can not add an element before an non-existing first element, right?

Finii commented 1 year ago

If num_elements < 1 the container is empty, right? And idx can not become negative? Then we could just return begin() without adding + idx, i guess. Since you can not add an element before an non-existing first element, right?

num_elements is the number of elements you want to add. The begin() + idx dance is to 'unconst' the pos iterator that we need to pass on :roll_eyes:

(I never liked the interface that mixes indices and iterators ;)

Finii commented 1 year ago

What I find smelly is this

const auto idx = static_cast<SizeType>(pos - begin());

pos is a const iterator, so I would use cbegin(), but that is another (possible) PR.

Finii commented 1 year ago

Oh you merged. I also do not really like this change and wanted to research into some better solutions, but then, would probably never have time for that :grimacing: