WW #12

[Sokolov Viacheslav]

╰─>$ make
mkdir -p obj
g++ -O2 -Wall -Werror -std=c++11 -Iinclude -c -MMD -o obj/main.o src/main.cpp
In file included from src/main.cpp:4:
include/my_vector_impl.h: In instantiation of ‘static my_vector::my_vector<T> my_vector::my_vector<T>::new_my_vector(size_t, size_t) [with T = int; size_t = long unsigned int]’:
include/my_vector_impl.h:105:44: required from ‘void my_vector::my_vector<T>::reserve(size_t) [with T = int; size_t = long unsigned int]’
include/tests.h:136:5: required from ‘void test_my_vector::test_reserve(const T&) [with T = int]’
include/tests.h:208:21: required from ‘void test_my_vector::test_my_vector(const T&, const T&) [with T = int]’
src/main.cpp:85:46: required from here
include/my_vector_impl.h:204:32: error: moving a local object in a return statement prevents copy elision [-Werror=pessimizing-move]

204 | return std::move(tmp_vector);

|

include/my_vector_impl.h:204:32: note: remove ‘std::move’ call
include/my_vector_impl.h: In instantiation of ‘static my_vector::my_vector<T> my_vector::my_vector<T>::new_my_vector(size_t, size_t) [with T = product::Product; size_t = long unsigned int]’:
include/my_vector_impl.h:105:44: required from ‘void my_vector::my_vector<T>::reserve(size_t) [with T = product::Product; size_t = long unsigned int]’
include/tests.h:136:5: required from ‘void test_my_vector::test_reserve(const T&) [with T = product::Product]’
include/tests.h:208:21: required from ‘void test_my_vector::test_my_vector(const T&, const T&) [with T = product::Product]’
src/main.cpp:86:121: required from here
include/my_vector_impl.h:204:32: error: moving a local object in a return statement prevents copy elision [-Werror=pessimizing-move]
include/my_vector_impl.h:204:32: note: remove ‘std::move’ call
cc1plus: all warnings being treated as errors
Makefile:17: recipe for target 'obj/main.o' failed
make: * [obj/main.o] Error 1

[Sokolov Viacheslav]

52 Product& Product::operator=(Product&& other) noexcept {
move удобно реализовывать через swap (хотя идеальный move - это просто move всех членов класса, для чего нужен тривиальный деструктор).

test_my_vector_default_consructible
непонятно, зачем отдельное пространство имен и отдельный набор тестов

test_my_vector
кажется, удобнее my_vector::test

fir, sec, vector_fir, vector_sec, vector_third
какой резон экономить 2-3 символа?

7 size_t checks_number = 0;
8 size_t failed_checks_number = 0;

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

190 std::string str_current;
191 out_current >> str_current;
192
193 std::string str_correct;
194 out_correct >> str_correct;
195
196 check(str_current == str_correct);
check(str_current.str() == str_correct.str())?

include/my_vector.h
возможно, стоит подключить в конце файла include/my_vector_impl.h, чтобы сценарий использования шаблонного кода не отличался от сценария использования нешаблонного (подключили заголовочный файл с объявлением и все работает).

40 friend std::ostream& operator<< <>(std::ostream&, const my_vector<T>&);

friend с шаблонами лучше не сочетать:
​https://habr.com/ru/post/472780/
​https://stackoverflow.com/questions/44267673/is-stateful-metaprogramming-ill-formed-yet
необходимости использовать friend в этом месте нет

9 size_t get_min_power_two(size_t n) {

здесь потенциально нарушется ODR. Компилируется только потому, что всего 1 единица компиляции. Непонятно, зачем эта функция в глобальном пространтсве имен. В общем случае зацикливается.

clear_and_delete_array
noexcept?

17 explicit my_vector();
noexcept?

37 new (&tmp_vector.array_[i]) T();
не проще push_back(T{})?

69 my_vector<T>& my_vector<T>::operator=(my_vector<T>&& other) noexcept {
проще реализовать через swap

123 my_vector<T> tmp_vector = new_my_vector(size_, count);
124 for (size_t i = 0; i < size_; i++) {
125 new (&tmp_vector.array_[i]) T(std::move(array_[i]));
126 }
reserve?

128 new (&tmp_vector.array_[i]) T(def);
129 tmp_vector.size_++;
push_back(def)?

199 void* data = aligned_alloc(alignof(my_vector<T>), sizeof(T) * new_capacity);
1) какая мотивация выравнивать?
2) почему alignof(my_vector<T>)?

Предложения по именованию.
get_min_power_two -> ceil_power_of_two
clear_and_delete_array -> destruct_and_free
new_my_vector -> allocate (_vector)
tmp_vector -> result
def -> default / t

[Sokolov Viacheslav]

здесь потенциально нарушется ODR. Компилируется только потому, что всего 1 единица компиляции. В общем случае зацикливается.

все еще актуально.
Предлагается:
1) вынести определение в .cpp
2) поставить assert, что n не слишком большое

Product
final / public virtual dtor

184 template <typename T> 185 std::ostream& operator<<(std::ostream& out, const my_vector<T>& vector) {
объявление все же стоит вынести к остальным объявлениям

29 void clear();
noexcept?

164 destruct_and_delete(array_, size_);
165 make_empty();
форматирование

[Sokolov Viacheslav]

135 template <typename T>
136 void my_vector<T>::push_back(const T& elem) {
137 if (size_ == capacity_) {
138 reserve(capacity_ + 1);
139 }
140 size_++;
141 new (&array_[size_ - 1]) T(elem);
142 }

по условию T(elem) может бросить исключение.
Наиболее простая корректная реализация:
push_back(T{elem});

120 my_vector<T> result = allocate_vector(0, count);
121 for (size_t i = 0; i < size_; i++) {
122 result.push_back(std::move(array_[i]));
123 }
124 for (size_t i = size_; i < count; i++) {
125 result.push_back(value);
126 }
127 *this = std::move(result);
по условию
125 result.push_back(value);
вот это может не получиться из-за проблем с копированием value.
Поэтому нужно поменять порядок операций, чтобы не забрать владение предыдущими элементами раньше времени.