WW_12

[Sokolov Viacheslav]

Кажется, часть файлов не залита.
#include "product.h"
но такого файла нет в репозитории.

Если я правильно понимаю, попытка была сделана прямо перед дедлайном?

[Jura Khudyakov]

Да, верно. Добавил product.h и product.cpp
Я уже точно не помню, судя по дате создания тикета - да, за час до. Хотя есть ещё коммит от 3 марта, пораньше. Посмотрел - в нём написан вектор, но что-то не компилится, и тестов нет.

После посылки я ещё что-то менял в коде, видимо, сам понял, что нужно исправить. Закоммитил сейчас это отдельным коммитом. Кажется, добавил noexcept, и конструктор копирования сделал через swap. И что-то в main поменял, но вроде ничего важного в main и не должно было быть, так как я вынес тесты отдельно в test и product

[Sokolov Viacheslav]

Общее резюме: версия довольно сырая. Везде, где есть проблемы, нужно написать тесты, исправление без теста будет оцениваться в половину. Попытку можете сделать еще одну, до конца модуля.

const методы не протестированы

в resize очевидные ошибки

15 my_vector(my_vector& other);

const

17 my_vector& operator=(my_vector& other);

const

22 void swap(my_vector&&) noexcept;

непонятно, зачем этот метод

29 void resize(size_t n, T t);

здесь будет лишняя копия, лучше принимать const T&. Два метода можно объединить в один:
void resize(size_t n, const T& t = T{})

46 my_vector_folder(my_vector_folder&&);

лучше бы noexcept, но еще лучше удалить (об этом ниже)

10 static Num up2pow(Num n) noexcept static, so outside namespace my_vector

непонятно, как это связано.
Возможно, лучше constexpr.
Само название на мой вкус лучше поменять, например, на ceilToPowerOf2. В текущем названии не отражено, степень чего.
Если метод шаблонный, то стоит поставить static_assert на то, какие типы на вход ожидаются (signed/unsigned, floating point? , ...)

15 Num k = 0;

почему Num?

18 return Num(1) << (k + 1);

может переполниться. Лучше Num{1}.

28 , array { static_cast<T*>(std::aligned_alloc(alignof(T), sizeof(T) * capacity)) }

какая мотивация использовать здесь aligned_alloc?

32 template <typename T>
33 my_vector<T>::my_vector_folder::~my_vector_folder()
34 {
35 std::free(array);
36 }

сейчас ответственность my_vector_folder не очень понятно в чем, зачем вообще эта обертка. Изначально предполагалось, что обертка нужна для удобной обработки исключений, а в этом деструкторе все хранимые объекты будут удалены.

118 template <typename T>
119 my_vector<T>::my_vector(size_t n, T t)
120 : folder { n }
121 {
122 size_t i = 0;
123 try {
124 for (; i < n; ++i)
125 new (folder.array + i) T(t);
126 } catch (...) {
127 for (; i > 0; --i)
128 delete (folder.array + (i - 1));
129 throw;
130 }
131 folder.size = n;
132 }
например, в этом месте не нужно было бы писать try-catch: folder уже сконструировать, и при возникновении исключения позовется его деструктор. Если поддерживать в нем актуальный size, то деструктор освободит всю заполненную память. Сейчас же один и тот же кусок кода многократно дублируется.

По этой причине лучше оставить только вот эту функцию удаления в случае исключения, а всю остальную логику перенести наружу. В таком случае copy- и move- конструкторы классу не нужны (с c++17).

90 template <typename T>
91 void my_vector<T>::my_vector_folder::shrink_to_fit() noexcept
92 {
93 capacity = std::min<size_t>(capacity, up2pow(size));
94 }

при этом реально аллоцировано больше памяти. Непонятно, зачем нужен метод в таком виде. Он ломает инвариант "capacity говорит, сколько памяти аллоцировано", а больше ничего не делает.

96 template <typename T>
97 my_vector<T>::my_vector()
98 : folder { 0 }
99 {

100 }
в текущей реализации не noexcept, но мог бы таким быть.

109 new (folder.array + i) T;
здесь объект будет инициализирован мусором. По условию просят инициализировать () / {}

119 my_vector<T>::my_vector(size_t n, T t)
значение t не используется, лучше принимать по const T&

46 array[i] = other.array[i];
Здесь происходит чтение неинициализированной памяти (в левой части). Здесь должен быть placement new.

62 array[i] = std::move(other.array[i]);

и здесь.

141 my_vector<T>::my_vector(my_vector&& other) noexcept
в текущей реализации не noexcept, потому что происходит (ненужная) аллокация памяти. Самая простая реализация - через noexcept ctor по-умолчанию и swap.

161 this->swap(std::move(other));
здесь можно просто swap(other)

305 if (folder.size == 0)
306 throw std::runtime_error("my_vector is empty");
с моей точки зрения предпочтительнее noexcept + assert, но это дело вкуса

289 template <typename T>
290 void my_vector<T>::push_back(const T& t)
291 {
292 reserve(folder.size + 1);
293 try {
294 new (folder.array + folder.size) T(t);
295 folder.size++;
296 } catch (...) {
297 folder.shrink_to_fit();
298 throw;
299 }
300 }
не соответствует strong exception safety: если при копировании T(t) произойдет исключение, поменяются адреса объектов, то есть пользователь заметит изменение.
Правильно делать так:

• сначала копия T(t)
• потом reserve
• потом placement new с std::move() - он noexcept

последние два шага можно выделить в push_back(T&&)

283 template <typename T>
284 const T& my_vector<T>::operator[](size_t index) const
285 {
286 return this[index];
287 }
это не компилируется и нарушает const-корректность

206 {
207 reserve(n);
208 if (folder.size <= n) {
209 while (folder.size > n)
210 (folder.array + folder.size - 1)->~T();
211 folder.size = n;
212 return;
213 }
214 size_t i = folder.size;
215 try {
216 for (; i < n; ++i) {
217 new (folder.array + i) T;
218 }
219 } catch (...) {
220 for (; i > folder.size; --i)
221 (folder.array + i - 1)->~T();
222 throw;
223 }
224 folder.size = n;
225 }

здесь очень много ошибок:

208 if (folder.size <= n) {
209 while (folder.size > n)
противоречат друг другу

209 while (folder.size > n)
210 (folder.array + folder.size - 1)->~T();
бесконечный цикл

217 new (folder.array + i) T;
не иницилизируется

не соответствуют strong exception safety: если в конструкторе T{} произойдет исключение, то пользователь увидит изменение capacity и адресов

корректно будет сделать независимый объект, заполнить его хвост, дальше std::move хранимых элементов в случае успеха и swap с этим временным объектом (как делается в reserve)

259 new (new_folder.array + i) T(folder.array[i]);
std::move

try-catch не нужен, потому что move - noexcept

[Jura Khudyakov]

Ух, я будто не свой код читал, столько в нём глупых ошибок. Не знаю, как так вышло. Очень надеюсь, что уменьшил их количество.

непонятно, как это связано.
ну, я хотел, чтобы метод используется только локально в этом файле, в нём же и оставался. Кажется, так всё таки делать не очень, и я занёс функцию в namespace my_vector

Если метод шаблонный, то стоит поставить static_assert на то, какие типы на вход ожидаются (signed/unsigned, floating point? , ...)
А лучше SFINAE :)
Но я просто убрал шаблонность, она здесь не очень-то нужна: тип всегда size_t

может переполниться
добавил assert, из-за этого убрал noexcept

какая мотивация использовать здесь aligned_alloc?
Кажется, и правда никакой. На лекции говорилось, что для атомарных переменных может быть полезен, но что впрочем и malloc достаточно хорош

my_vector()

в текущей реализации не noexcept, но мог бы таким быть.

Добавил noexcept конструктор folder по умолчанию. Можно было бы и немного покостылять конструктор от size_t, но так получатся более явно и красиво

противоречат друг другу
ага, там в другую сторону. Открыл для себя, что resize(n, x) с n < size в std::vector просто уменьшает, и x никак не используется

с моей точки зрения предпочтительнее noexcept + assert, но это дело вкуса
мне кажется несколько неправильным говорить, что функция noexcept, а потом оттуда кидаться assert-ами. Хотя, кажется, в стандартной библиотеке так и делают

Вроде много чего дополнительно протестировал, надеюсь, ничего не забыл, писал ночью :) старые тесты тоже оставил
Я б ещё использовал доктест, но раз старые тесты написаны ручками, пускай и остальные тоже будут.

[Sokolov Viacheslav]

Наверное, вместо folder подразумевалось holder.

Не смотря на удобство реализации

67 my_vector<T>::my_vector(size_t n)

через

73 my_vector<T>::my_vector(size_t n, T t)

так все же делать некорректно, потому что требования на default ctor и на copy ctor могут отличаться. Например, copy ctor может отсутствовать.

136 for (size_t tail_index = folder.size; tail_index < n; ++tail_index)
137 new (new_folder.array + tail_index) T{t};
если здесь произойдет исключение, деструктор не отработает корректно, потому что в нем предполагается, что все size элементов заполнены (а size в этот момент 0)

Ух, я будто не свой код читал

иногда полезно почитать свой старый код :)

179 assert(noexcept(vector<T>{}));
лучше static_assert

170 return const_cast<T&>(
171 static_cast<const std::remove_reference_t<decltype(*this)>&>(*this)[index]);
172 is this ok? :)
nope. Дурацкое место в языке, приходится дублировать код.

добавил assert, из-за этого убрал noexcept

assert не противоречит noexcept. noexcept только про исключения. assert гарантирует аварийное завершение программы. Это разные механизмы.

Если метод шаблонный, то стоит поставить static_assert на то, какие типы на вход ожидаются (signed/unsigned, floating point? , ...)
А лучше SFINAE :)

SFINAE хуже, потому что от static_assert получается явное сообщение об ошибке, а SFINAE может маскировать проблему. Без static_assert, к сожалению, stacktrace от gcc часто выглядит совершенно непонятно.

ну, я хотел, чтобы метод используется только локально в этом файле, в нём же и оставался. Кажется, так всё таки делать не очень, и я занёс функцию в namespace my_vector

в случае шаблонов понятия "локально" не существует