ryzhikovas Jun 17 at 18:15

UB or not UB: дублируем элемент std::vector

Medium

10 min

6.7K

Programming*Perfect code*C++*

+54

Comments 15

Panzerschrek Jun 18 at 09:19

auto x = v.back();
v.push_back(std::move(x));

Чем такой вариант не устраивает? Он 100% рабочий и не надо ломать голову, UB это или нет.

inemiten Jun 18 at 09:42

std::move(x) в этом примере не имеет никакого смысла. Перемещения не произойдет.

csl Jun 18 at 10:08

Из-за особенностей жизненного цикла?

inemiten Jun 18 at 10:25

Если отвечать формально, то из-за правил referece collapsing: https://www.ibm.com/docs/en/xl-c-and-cpp-aix/16.1?topic=operators-reference-collapsing-c11

Фактически сейчас в примере написан вот такой код (запись в псевдокоде):

v.push_back((const &)(&&)(x));

Что эквивалентно просто:

v.push_back((const &)(x));

ZirakZigil Jun 18 at 10:42

push_back'ов два, const T& и T&&. std::move(x) даёт T&&, идеальным кандидатом которому будет именно push_back(T&&)

inemiten Jun 18 at 10:45

А, действительно, вы правы, спасибо. Проглядел, что там же просто auto x, что создаст копию объекта.

ryzhikovas Jun 18 at 10:09

Рабочий, но не соответствует исходному. Вы именно перемещаете элемент из вектора (и конструируете копию в конце). В результате выполнения такого кода vec.front() == nullptr:

std::vector vec = {std::make_shared<int>(42)};
vec.push_back(std::move(vec.back()));

Исходный пример действительно можно упростить, явно создавая копию, например, так:
vec.push_back(auto(vec.back()));
Но хотелось именно поломать голову)

Panzerschrek Jun 18 at 17:12

Первая строка делает копию - back возвращает ссылку, которая используемый для создания копии в auto переменной.
Вторая строка реализует push_back с перемещением, это гарантируется тем, что std::move возвращает && ссылку.

Строго говоря это код не эквивалентен описанному в статье, но практически малоотличим и лучше такое использовать, чтобы UB обходить подальше.

ryzhikovas Jun 18 at 17:40

Вы совершенно правы. У меня "глаз замылился" и прочитал первую строку так: auto& x = v.back();

Abstraction Jun 18 at 10:01

И на первый взгляд этого достаточно, чтобы уверенно заявить: код выше приведет к UB, если к моменту вызова push_back выполняется утверждение size() == capacity().

Нет. Код выше эквивалентен:

int& ref = v.back();
//в этот момент ref - валидная ссылка
v.push_back(ref); //А _после_ вызова - невалидная

Понятие "после" (sequenced-after) довольно хорошо формализовано в стандарте.

То что бедной реализации необходимо учесть что валидная ссылка может быть и на память, прямо или косвенно управляемую this - проблемы этой реализации, точно так же как случай присваивания объекта самому себе в operator=() и тому подобные штуки.

ryzhikovas Jun 18 at 10:59

Спасибо, к похожему выводу в конечном счете и пришел. Да, из правил последовательности выполнения следует, что получение ссылки ref происходит строго до выполнения метода push_back. А вот что "бедной реализации необходимо учесть" - было неочевидно, так как допускал трактовку в духе "если передают ссылку на свой элемент, он погибает при реаллокации → UB → такая ситуация в валидном коде невозможна" . К сайд-эффектам с "атомарным выполнением" из п.7 наш случай вроде бы не относится.

Dasfex Jun 20 at 00:58

Есть замечательный лайтнинг с похожей тематикой: My favourite memory leak (https://www.youtube.com/watch?v=LKKmPAQFNgE).

cher-nov Jul 23 at 12:11

Спасибо, интересный вопрос подняли - придётся поправить это в Gena. Но увы, написана статья весьма скверно, мысль по ней скачет как ошалелая.

В частности, сильно обескуражили (и стоили мне часа три раздумий в фоне) вот эти пассажи:

Готово, написали корректную реализацию, которая приводит к UB в интересующем случае?

"Корректная" реализация приводить к UB не может по определению. Я понимаю, что Вы намеренно писали так, чтобы "подвоха" имелась при соблюдении буквы стандарта, но здесь выбрано не то слово.

Оговорка для случая, не относящегося к нашей перегрузке push_back (когда копирующий конструктор недоступен, а перемещающий выбрасывает исключение), не очень интересна. А вот первое требование фатально нарушено.

Тут до меня смысл сказанного добрался уже далеко не так быстро. Если по порядку, то:

"Первое требование" это какое? В цитате из стандарта, здесь упомянутой, непонятен порядок следования and и or, поэтому уразуметь под "первым" тут можно разные вещи:
- If an exception is thrown while inserting a single element at the end and T is Cpp17CopyInsertable or is_nothrow_move_constructible_v is true
- If an exception is thrown while inserting a single element at the end and T is Cpp17CopyInsertable
- If an exception is thrown while inserting a single element at the end
- T is Cpp17CopyInsertable (поскольку идёт первым в or-подвыражении)
Про очерёдность логических операций и устранение её неоднозначности (скобками в арифметике, запятыми в грамматике) эти славные люди, видимо, ни сном ни духом.
Почему читатель должен счесть это требование нарушенным? Ваша статья описывает случай "while inserting a single element at the end" начиная прямо с заголовка, и пришлось пролистать её до конца, дабы хоть как-то понять, что здесь имеется в виду не вставка элемента в конец как таковая, а сам момент его помещения в новое место в памяти. Который потому и должен происходить до перекладывания остальных элементов, а не после.

Возможно безопасность исходного примера неявно следует из этого утверждение?

Чтобы было понятно, о каком "исходном примере" здесь идёт речь, это предложение должно начинать собой новый абзац.

Объект _ при завершении своей жизни (выход из области 3-14)

Он и при выходе из функции уничтожился бы. Зачем там лишняя область видимости? Не говоря уже о том, чем это лучше обычного try {} catch.

И так далее. Было очень тяжело читать, извините.

ryzhikovas Jul 23 at 13:26

Спасибо за столь развернутый фидбек.
Попробую ответить на поднятые вопросы, буду рад, если это упростит понимание.

"Корректная" реализация приводить к UB не может по определению

Подразумевал, что вызывая корректную реализацию можем получить UB, если используем ее недопустимым образом.

"Первое требование" это какое?

Речь про первое предложение из приведенной в тексте статьи цитаты: If an exception is thrown while inserting a single element at the end and T is Cpp17CopyInsertable or is_nothrow_move_constructible_v is true, there are no effects.

Почему читатель должен счесть это требование нарушенным?

Эту мысль раскрываю начиная с предложения "Что сейчас может пойти не так?". И согласен, здесь порядок изложения неудачен.

Зачем там лишняя область видимости? Не говоря уже о том, чем это лучше обычного try {} catch

Действительно, после пережитого перед публикацией рефакторинга scope лишний. Спасибо, исправлю. Вариант с try-catch многословнее, т.к. deallocate придется вызвать в двух ветках: основной, когда исключение не вылетело, и в блоке catch.

cher-nov Jul 23 at 14:43

Вариант с try-catch многословнее, т.к. deallocate придется вызвать в двух ветках: основной, когда исключение не вылетело, и в блоке catch.

Вызовы deallocate здесь подразумевают разные параметры (в случае ошибки - новый буфер, в случае успеха - старый), что Вы маскируете через std::swap (и заодно ещё зачем-то обмениваете _capacity с next_capacity вместо простого присваивания). Не, я понимаю, конечно, что в C++ нет finally до сих пор, а deref удобнее, но в примерах важнее всего простота и наглядность.