WW #12

[Артур Гулецкий (huletski)]

Замечания:

• не компилируется такой код:

  my_vector::my_vector<int> v1, v2;
  v1 = std::move(v2);
  

• если при вызове resize capacity у вектора больше чем нужно, перевыделять память не надо:

  my_vector::my_vector<int> vi(6);
  assert(vi.capacity() == 8);
  vi.resize(3);
  assert(vi.capacity() == 8);
  

• std::move не используется там, где мог бы (e.g. resize, т.к. move ctor/op= у Т по условию noexcept);

my_vector.h

• 18: точно все модификаторы указаны?
• 41: почему во множественном числе?
• 46: virtual избыточен;

my_vector_impl.h

• 2: s/math.h/cmath
• 49, 63: гарантии: неясно, кто освободит память, выделенную под _array и уже инициализированные элементы, в случае исключения;
• 52: (fyi) в общем случае лучше позволять T-ctor-specific исключению покинуть метод, т.к. описание конкретной проблемы от Т лучше, чем "не в силах" от контейнера;
• 87: std::swap выглядит подозрительно;
• 92: вместо таких "хаков" сделайте приватный конструктор/метод, который принимает capacity и возвращает вектор, либо напишите веселый коммент, который обращает внимание на и объясняет использующийся "хак";
• 142: минус строгие гарантии. copy ctor в общем случае может бросить исключение после того, как capacity изменилось (в результате вызова reserve);
• 142: (_array + size++) проще выглядит;
• дублирование логики инициализации/копирования/удаления элементов в интервале массива;

main.cpp

• 10: нарушение правила трех (пяти);
• 90: старайтесь не ставить скобки там, где они не обязательны и не улучшают читабельность, т.к. такие скобки добавляют visual noise -> -readability.

---

Баллы: 3.5, доделывайте.

[Артур Гулецкий (huletski)]

Не работают ваши же тесты:

{lab_12}[2217]$ pwd && svn up && svn status
/home/hfx/dvl/cpp19/gabitov.daniil/lab_12
Updating '.':
At revision 3728.
{lab_12}[2218]$ make
mkdir obj
g++ -c -std=c++17 -Wall -Wall -Wextra -Werror -Iinclude  src/main.cpp -o obj/main.o 
g++ obj/main.o -o lab_12
{lab_12}[2219]$ valgrind ./lab_12 
==18310== Memcheck, a memory error detector
==18310== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==18310== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==18310== Command: ./lab_12
==18310== 
**18310** new/new[] failed and should throw an exception, but Valgrind
**18310**    cannot throw exceptions and so is aborting instead.  Sorry.
==18310==    at 0x4C2D96C: ??? (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18310==    by 0x4C2E885: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18310==    by 0x404899: my_vector::my_vector<int>::my_vector(my_vector::my_vector<int> const&) (in /home/hfx/dvl/cpp19/gabitov.daniil/lab_12/lab_12)
==18310==    by 0x404991: my_vector::my_vector<int>::reserve(unsigned long) (in /home/hfx/dvl/cpp19/gabitov.daniil/lab_12/lab_12)
==18310==    by 0x403B65: my_vector::my_vector<int>::resize(unsigned long) (in /home/hfx/dvl/cpp19/gabitov.daniil/lab_12/lab_12)
==18310==    by 0x40246F: void test::test_resize<int>(int, int) (in /home/hfx/dvl/cpp19/gabitov.daniil/lab_12/lab_12)
==18310==    by 0x401E1A: void test::test_my_vector_default_constructible<int>(int, int) (in /home/hfx/dvl/cpp19/gabitov.daniil/lab_12/lab_12)
==18310==    by 0x401AE0: main (in /home/hfx/dvl/cpp19/gabitov.daniil/lab_12/lab_12)
==18310== 
==18310== HEAP SUMMARY:
==18310==     in use at exit: 72,800 bytes in 4 blocks
==18310==   total heap usage: 9 allocs, 5 frees, 72,904 bytes allocated
==18310== 
==18310== LEAK SUMMARY:
==18310==    definitely lost: 0 bytes in 0 blocks
==18310==    indirectly lost: 0 bytes in 0 blocks
==18310==      possibly lost: 0 bytes in 0 blocks
==18310==    still reachable: 72,800 bytes in 4 blocks
==18310==         suppressed: 0 bytes in 0 blocks
==18310== Rerun with --leak-check=full to see details of leaked memory
==18310== 
==18310== For counts of detected and suppressed errors, rerun with: -v
==18310== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Основные замечания:

• падает такой код

  my_vector::my_vector<int> v;
  v.resize(10);
  v.resize(3);
  assert(v.size() == 3);
  

• op<<: лишний пробел выводится после последнего элемента;
• гарантии, см. ниже.
• Все еще: std::move не используется там, где мог бы (e.g. resize, т.к. move ctor/op= у Т по условию noexcept);

my_vector_impl.h

• 7: почему переменная глобальная, а не локальная? В имени опечатка;
• my_vector::my_vector -гарантии: неясно, кто освободит память, выделенную под _array;
• 110: лол, слишком буквально. Внимание я обратил, с этим справились, но объяснения "хака" так и не увидел. Нужно объяснение или реализовать метод как-то иначе (см. пример в предыдущем разборе);ё
• Все еще: дублирование логики инициализации/копирования/удаления элементов в интервале массива;

main.cpp

• 22: комментарий вводит в заблуждение: указатель не владеет строкой (иначе почему в деструкторе строка не освобождается). Нужно либо реализовать владение (что подразумевалось по условию), либо изменить комментарий.

---

Баллы: 4, доделывайте. Если будете присылать исправления, напишите, пожалуйста, какие замечания починили.

[gabitov.daniil]

Постарался исправить все.

[Артур Гулецкий (huletski)]

Проверялась версия 3822.

Замечания:

• теперь падает такой код:

  my_vector::my_vector<int> v;
  std::cout << v;
  

• my_vector::resize не работает на нетривиальных классах (т.е. на Product'e): последний аргумент в 146 должен быть new_vector._array, а не _array;
• std::move в resize нужно было использовать более эффективно: создать новый вектор/буфер нужного размера, после чего переместить в него все данные поэлементно из текущего (безопасно с т.з. исключению, т.к. move op= у Т по условию noexcept) и свопнуть this с новым вектором, куда переместились элементы (тоже noexcept операция). Аналогично для reserve;
• my_vector::my_vector(std::size_t): грабли, связанные с глобальной переменной и гарантиями, нашли вас: если вылетело исключение, счетчик успешных аллокаций не сбрасывается -> если в конструкторе вылетело исключение при создании ненулевого элемента, использовать вектор в программе больше нельзя (в случае повторной обработки исключения, счетчик будет хранить неверное значение). Мораль: не используйте глобальные переменные, если этого можно избежать; в данном случае, глобальная переменная -- часть состояния, которое нужно откатывать. Навскидку, избавиться от глобальной переменной можно было бы одним из следующих способов:
  • создать приватный класс, в методах которого реализованы примитивные операции над элементами (инициализация, удаление, копирование, перемещение по интервалу) со строгими гарантиями исключений, глобальная переменная-счетчик стала бы полем класса;
  • либо передавать ссылку на локальную переменную-счетчик как аргумент методу create_array_el;
  • либо смириться с дублированием и не выносить код инициализации в отдельную функцию, мотивировав тем, что усложнение кода отслеживания количества успешно созданных элементов хуже дублирования с т.з. ясности кода (эту мотивацию нужно было бы написать в комментарии в коде).

my_vector_impl.h

• 2: s/math.h/cmath

• 167: копию можно переместить, а не скопировать: new ... T(std::move(t));

main.cpp

22: комментарий вводит в заблуждение: указатель не владеет строкой (иначе почему в деструкторе строка не освобождается). Нужно либо реализовать владение (что подразумевалось по условию), либо изменить комментарий.

---

Баллы: 5.2.