On Mon, 24 Mar 2025 at 12:40, Tomasz Kaminski wrote:
> As a note reserving for empty vector:
> + // If there are no existing elements it's safe to allocate now.
> + if (__sz == 0)
> + reserve(__n);
> Will invalidate v.begin(), v.end(), so if the incoming range stored these
> iterators as a range, and uses it internally, this will be broken.
> If these iterator are saved before the call, this does not run into
> re-entrancy.
> We could change that to look if capacity is 0 (to optimize default_construct
> + append case), but I am fine with not supporting this case.
Good catch, e.g. this would have undefined behaviour:
#define _GLIBCXX_DEBUG
#include <vector>
#include <ranges>
int main()
{
std::vector<int> v1(1);
std::vector<int> v2(20);
v1.clear();
std::ranges::subrange r1(v1.begin(), v1.end());
std::ranges::subrange r2(v2.begin(), v2.end());
v1.append_range(std::views::concat(r2, r1));
}
r1 contains valid iterators to the empty v1, then append_range does
reserve(20) and invalidates r1.
It doesn't fail in debug mode because
__gnu_debug::vector::append_range doesn't invalidate all existing
iterators until *after* calling Base::append_range, so when the
reserve(20) call invalidates r1, the debug iterators inside r1 don't
get invalidated immediately. So when we iterate over the concat_view,
r1.begin()==r1.end() and we don't notice we're using invalid
iterators.
And it doesn't fail for constexpr either, because of PR119426. So I
don't currently have a test that fails due to the bug noted above.
I'll fix it, and maybe try to improve
__gnu_debug::vector::append_range to invalidate iterators before
calling Base::append_range.
