HW #3

[abramov.nikita]

Пока что реализовано только дерево Хаффмана

[Egor Suvorov]

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

Корректность 0/9 из-за отсутствия main

• Проблемы с компиляцией:

  osboxes@osboxes:~/cpp2019/cpp19/abramov.nikita/hw_03$ make
  mkdir -p bin
  clang++-10 -O2 -Wall -Werror -std=c++17 -Iinclude -stdlib=libc++ -c -MMD -o bin/bin_manip.o src/bin_manip.cpp
  clang++-10 -O2 -Wall -Werror -std=c++17 -Iinclude -stdlib=libc++ -c -MMD -o bin/HuffmanArchiver.o src/HuffmanArchiver.cpp
  In file included from src/HuffmanArchiver.cpp:1:
  In file included from include/HuffmanArchiver.h:4:
  In file included from include/Huffman.h:7:
  /usr/lib/llvm-10/bin/../include/c++/v1/set:602:30: error: the specified comparator type does not provide a viable const call operator [-Werror,-Wuser-defined-warnings]
          static_assert(sizeof(__diagnose_non_const_comparator<_Key, _Compare>()), "");
                               ^
  include/Huffman.h:15:7: note: in instantiation of member function 'std::__1::set<huffman_tree::HuffmanTree::HuffmanNode, huffman_tree::HuffmanTree::less_compare, std::__1::allocator<huffman_tree::HuffmanTree::HuffmanNode> >::~set' requested here
  class HuffmanTree {
        ^
  /usr/lib/llvm-10/bin/../include/c++/v1/memory:2618:7: note: in instantiation of member function 'std::__1::default_delete<huffman_tree::HuffmanTree>::operator()' requested here
        __ptr_.second()(__tmp);
        ^
  /usr/lib/llvm-10/bin/../include/c++/v1/memory:2572:19: note: in instantiation of member function 'std::__1::unique_ptr<huffman_tree::HuffmanTree, std::__1::default_delete<huffman_tree::HuffmanTree> >::reset' requested here
    ~unique_ptr() { reset(); }
                    ^
  src/HuffmanArchiver.cpp:40:17: note: in instantiation of member function 'std::__1::unique_ptr<huffman_tree::HuffmanTree, std::__1::default_delete<huffman_tree::HuffmanTree> >::~unique_ptr' requested here
      auto tree = std::make_unique<huffman_tree::HuffmanTree>(symbol_to_frequency); 
                  ^
  /usr/lib/llvm-10/bin/../include/c++/v1/__tree:976:5: note: from 'diagnose_if' attribute on '__diagnose_non_const_comparator<huffman_tree::HuffmanTree::HuffmanNode, huffman_tree::HuffmanTree::less_compare>':
      _LIBCPP_DIAGNOSE_WARNING(!std::__invokable<_Compare const&, _Tp const&, _Tp const&>::value,
      ^                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /usr/lib/llvm-10/bin/../include/c++/v1/__config:1297:21: note: expanded from macro '_LIBCPP_DIAGNOSE_WARNING'
       __attribute__((diagnose_if(__VA_ARGS__, "warning")))
                      ^           ~~~~~~~~~~~
  1 error generated.
  Makefile:17: recipe for target 'bin/HuffmanArchiver.o' failed
  make: *** [bin/HuffmanArchiver.o] Error 1
  

Потом:

    osboxes@osboxes:~/cpp2019/cpp19/abramov.nikita/hw_03$ make
clang++-10 -O2 -Wall -Werror -std=c++17 -Iinclude -stdlib=libc++ -c -MMD -o bin/HuffmanArchiver.o src/HuffmanArchiver.cpp
clang++-10 -O2 -Wall -Werror -std=c++17 -Iinclude -stdlib=libc++ -c -MMD -o bin/Huffman.o src/Huffman.cpp
src/Huffman.cpp:52:35: error: result of comparison of constant 256 with expression of type 'unsigned char' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
    for (unsigned char ch = 0; ch < ASCII_MAX; ch++) {
                               ~~ ^ ~~~~~~~~~
1 error generated.
Makefile:17: recipe for target 'bin/Huffman.o' failed
make: *** [bin/Huffman.o] Error 1

Тесты 0/8

Архитектура 1/5

• CLI
  • Зачем деструктор? Правило нуля.
• HuffmanArchiver
  • Кажется, кроме compress/extract публичных методов тут быть не должно.
  • Неясно, почему read_table принимает ostream, как и write_table
  • Неясно, зачем write_text принимать int32_t.
• Выберите из int32_t, uint32_t, int ровно один тип на каждый случай (а лучше просто один) и исползуйте. Сейчас рандом какой-то. Аналогично с char/uint8_t/int8_t.
• Статистика по сжатию — отдельный объект со своей жизнью.
• Если делать только с обычными потоками ввода-вывода, вы наверняка помрёте, когда надо будет паковать биты. Придумайте тут какую-нибудь хорошую абстракцию.
• Huffman
  • Конструктор — explicit, класс — final, нужны noexcept.
  • std::string symbol — оксюморон. std::optional<char>?
  • HuffmanNode — используйте правило нуля.
  • Ужасное заглушение ошибок в конструкторе HuffmanTree: странный if (size == 1) без пояснений.
• Выберите

Стиль 1/8 (мало корректности, чтобы ставить больше за стиль)

• Не basic_istream/basic_ostream, а istream/ostream
• Не используйте чистые указатели, если можете. Используйте ссылки. Например, в HuffmanArchiver. Исчезнут assert(in && out);.
• Лучше не int32_t, а просто int.
• ASCII_MAX — это не ASCII ни разу. ASCII — от 0 до 127. А ещё это не про ASCII на самом деле, а про какое-то свойство вашего алгоритма Хаффмана. Если очень хочется обобщать, то используйте стандартный CHAR_BIT, который на всех разумных системах равен 8. Или поставьте один раз static_assert(CHAR_BIT == 8) и дальше везде 8, 256.
• write_table: тут можно не заводить переменную c, просто static_cast в нужных местах.
• Инициализировать unique_ptr не требуется, он и так nullptr.
• get_left/get_right пусть лучше возвращают ссылки. const unique_ptr<>& бесполезен и является лишним ограничением на возвращаемое значение.
• less_compare — less compare что?
• get_codes_from_tree — снова не нужен unique_ptr. И это, часом, не метод вершины?
• queue_ — не поле, используется отлько в конструкторе.
• Поехали отступы. Пройдитесь автоформатированием.
• Используйте member initializer list для конструкторов, а не присваивания внутри. Например, у HuffmanNode.
• Внутри последнего ифа в HuffmanTree написано дублирование кода get_codes_from_tree.

[Egor Suvorov]

/
Проверялась ревизия 4014 или ранее.

Третья попытка разблокирована.

Корректность 7/9

• Не поддерживается флаг --output
• Файлы не открываются в бинарном режиме => не работает под Windows.

Тесты 4.5/8

• Не протестирован парсинг аргументов.
• Вместо ручной проверки .size() и [0] лучше сделайте == vector<bool>{0, 1, 0, 1, 0}
• Проверьте, пожалуйста, точные коды построенных символов. Сейчас у вас префиксность не проверяется.
• Лучше тестировать не при помощи чтения из stringstream, а сравнением str() с заготовленным массивом/вектором при помощи == и конструктором string(first, last).
• Не протестирован битовый ввод-вывод в отрыве от всего остального.

Архитектура 2.5/5 (основная проблема выделена полужирным)

• Не хватает namespaces для bin_manip и CLI. Хотя бы для MODE, режимов очень много разных. А лучше во всех файлах как-то разделить.
• CLI
  • ArgParseException
    • Лучше отнаследовать от logic_error/runtime_error, тогда реализация займёт пару строк из-за using`.
    • Не выполняется правило пяти.
    • Не используйте throw(), максимум noexcept().
  • CLI::get_file() — не самая удачная идея, надо запоминать, какой файл где. Плюс это скорее get_files(). Лучше отдельный геттер.
• Huffman
  • HuffmanNode
    • Не выполнено правило пяти. Прям совсем.
    • Конструктор по умолчанию не нужен. И тест совершенно не проясняет, зачем он добавлен.
  • HuffmanTree
    • get_min_node — вообще не метод, а приватная функция для Huffman.cpp.
    • get_codes лучше возвращать по константной ссылке.
• HuffmanArchiver
  • HuffmanStat
    • Не print(), а operator<<. cout ничем не знаменит.
  • HuffmanArchiver
    • Много за что отвечает: и подсчёт количества символов, и форматирование таблицы, и сжатие/разжатие. Это видно по количеству приватных методов.
    • В частности, read_table/write_table — не методы, а свободные функции. А судя по тому, что они делают — это вообще методы отдельной сущности "таблица частот".
• main
  • Зачем-то делает кусок работы CLI, проверяя количество аргументов.

Стиль 3/8 (полужирным выделены адские замечания, каждое само по себе на -1)

• Makefile — жесть. Лучше переписать на CMake или хотя бы воспользоваться переменными, чтобы не приходилось руками заменять g++ на clang++-10 -stdlib=libc++ и/или добавлять -fsanitize=address
• bit_manip
  • ОБЪЯВЛЯЙТЕ ПЕРЕМЕННЫЕ В ЦИКЛЕ (много где актуально).
  • НЕ ИСПОЛЬЗУЙТЕ C-STYLE CAST.
  • Не basic_ostream<char>, а ostream.
  • Незачем лишний уровень индирекции в write_le_int32/read_le_int32 — там можно сразу сделать friend operator<< без промежуточного ().
  • Не хватает заголовка для int32_t
  • Один из операторов почему-то принимает манипулятор по значению, а не по ссылке.
• CLI
  • u/c — плохие названия для режима.
  • ArgParseException
    • неэффективно копирут себе std::string: принимайте по значению и делайте move.
    • Должен быть final и много где noexcept
  • CLI
    • Возврат копии std::string — не noexcept, возможны исключения
    • MODE --> Mode.
    • Переменная size_t i должна быть объявлена внутри цикла. А цикл должен быть for.
    • Не надо проверять количество аргументов и вываливаться с непонятной ошибкой "мало аргументов", а честно пробегаться по ним и проверять, чего не хватает. Вы в любом случае это делаете, так что просто убираем лишний if.
    • Не strcmp, а просто ==, раз уж std::string.
    • Не надо пытаться открывать файл для чтения прямо в парсинге аргументов. У вас всё равно может возникнуть проблема с открытием файла чуть попозже, только так вы этот путь кода замаскируете.
    • Нехорошо, что не проверяется, что аргументы корректны (нет дубликатов и противоречий).
• Huffman
  • Не <limits.h>, а <climits>
  • HuffmanNode
    • Используйте member initialization list, а не присваивания.
    • Странная расстановка пустых строк в зщаголовке.
    • get_left/get_right — noexcept, равно как и конструкторы.
  • HuffmanTree
    • get_min_node --> extract_min_node.
    • get_codes_from_tree: очень странный инвариант из-за лишнего параметра direction. Пусть лучше этим занимается вызывающая сторона. Заодно можно будет соптимизировать и не копировать каждый раз current_code. Всё равно этот метод скрыт в публичной обёртке.
    • Методы объявляются в .cpp и в .h в разном порядке.
    • Счётчик цикла по умолчанию — int, а не uint32_t
    • А что происходит, если дерево всё-таки пустое? Сейчас ощущение, что ошибка заглушается.
    • Переменная new_node не нужна.
    • Есть странная пустая строка после if (!root_->is_leaf()) {
    • А ещё там же очень странно, что вообще приходится какие-то случаи разбирать. Такие же разбираются внутри get_codes_from_tree.
• HuffmanArchiver
  • HuffmanStat
    • start_size/end_size — странные названия. input/output лучше.
  • HuffmanArchiver
    • Поле лучше инициализировать при помощи {}, а не Foo x = Foo(....)
    • numb_of_elem — непонятное название. Может, "длина закодированного/декодированного текста в символах" или что-то покороче?
    • Раз уж разбираете крайний случай "пустой файл", можно и таблицу не выводить.
    • write_orig_text/write_compress_text — дико странные названия.
    • А ещё эти две функции выглядят очень жутко: состояние из шести переменных, названия i/j/ch/ch1, сишный лапшекод. Даже не хочу читать.
  • В тестах
    • Не переиспользуйте переменные в тестах.
    • tmp_out/res_out — есть гораздо более подходящие названия. Например, что именно содержится в tmp_out?
• main
  • Ловите исключения по константной ссылке
  • Не open, а вызов конструктора.

[abramov.nikita]

Дедлайн третьей попытки - 03.05 21:59 (см переписку в телеграмме)

[Egor Suvorov]

Тикет потерялся.