HW #3

[Egor Suvorov]

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

1. Местами плохо компилируется при помощи clang/libc++:

   osboxes@osboxes:~/cpp2019/cpp19/surkov.petr/hw_03$ make
   mkdir obj
   clang++-10 -O3 -c -Wall -Wextra -Werror -Iinclude -std=c++17 -stdlib=libc++ src/HuffmanIOProcessing.cpp -o obj/HuffmanIOProcessing.o
   clang++-10 -O3 -c -Wall -Wextra -Werror -Iinclude -std=c++17 -stdlib=libc++ src/Huffman.cpp -o obj/Huffman.o
   src/Huffman.cpp:45:18: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
           q.insert(std::move(pair{p.second, new HuffmanTree(p.first)}));
                    ^
   src/Huffman.cpp:45:18: note: remove std::move call here
           q.insert(std::move(pair{p.second, new HuffmanTree(p.first)}));
                    ^~~~~~~~~~                                        ~
   src/Huffman.cpp:51:18: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
           q.insert(std::move(pair{p1.first + p2.first,
                    ^
   src/Huffman.cpp:51:18: note: remove std::move call here
           q.insert(std::move(pair{p1.first + p2.first,
                    ^~~~~~~~~~
   2 errors generated.
   Makefile:31: recipe for target 'obj/Huffman.o' failed
   make: *** [obj/Huffman.o] Error 1
   

Потом не хватает <string> в include/CLI.hpp, из-за этого тоже ошибка компиляции.
Потом:

test/TestHuffmanIOProcessing.cpp:17:16: error: implicit conversion from 'int' to 'std::__1::basic_ostream<char, std::__1::char_traits<char> >::char_type' (aka 'char') changes value from 255 to -1 [-Werror,-Wconstant-conversion]
        is.put(255);
           ~~~ ^~~
test/TestHuffmanIOProcessing.cpp:86:69: error: implicit conversion from 'int' to 'std::__1::basic_ostream<char, std::__1::char_traits<char> >::char_type' (aka 'char') changes value from 245 to -11 [-Werror,-Wconstant-conversion]
        is.put((1 << 7) + (1 << 6) + (1 << 5) + (1 << 4) + (1 << 2) + 1);
           ~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
test/TestHuffmanIOProcessing.cpp:87:47: error: implicit conversion from 'int' to 'std::__1::basic_ostream<char, std::__1::char_traits<char> >::char_type' (aka 'char') changes value from 153 to -103 [-Werror,-Wconstant-conversion]
        is.put((1 << 7) + (1 << 4) + (1 << 3) + 1);
           ~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
3 errors generated.

2. На одном из минимальных тестов ошибка: "Test failed -- invalid compressed size reported: 12 instead of 13". Может, что-то не так с какими-нибудь округлениями, если вы биты считаете? Стабильно на некоторых тестах выводится размер сжатого текста на единицу меньше, чем надо на самом деле.

Тесты 7/8

• CLI
  • Лучше не пустые фигурные скобки внутри TEST_CASE, а SUBCASE. Куски-то всё-таки независимые, надо уметь их независимо проверять.
• IOProcessing
  • Не надо делать put в stringstream руками, создайте его от нужного string{....}.
  • Не надо заводить промежуточный bool b;, просто CHECK(es.getByte())
  • Используйте бинарные литералы: 0b00000111 == (1 << 2) | (1 << 1) | (1 << 0).
• Не ловят баг из корректности.
• Huffman
  • Сделайте using для unordered_map<....>, можно даже внутри HuffmanTree.
  • Не проверяются точные построенные коды. А это важно: например, они должны быть префиксными.
  • Лучше проверять просто на == с точным кодированием.
  • Нет большого теста на какую-нибудь оптимальность кодов или что-то такое.

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

• CLI
  • CLIException — final и лучше от runtime_error/logic_error, конструкторы проще станут заодно.
  • const-поле — на самом деле как минимум статическое, а лучше — деталь реализации (потому что private static), должна быть в .cpp. А ещё это явное дублирование кода, надо избавиться.
  • Мне не очень нравится, как устроен разбор. Много случаев. Посмотрите, как в общем случае описывает свои аргументы, скажем, библиотека argparse в Python.
• Битовые строчки — точно не std::string. Заодно ассёрты исчезнут.
• HuffmanIOProcessing:
  • Скорее у вас не Stream (это обычно один однонаправленный поток), а IO: EncodingIO, DecodingIO.
  • В целом нет ощущения целостности архитектуры. Кажется, что вам потребовались какие-то методы — вы их как-то реализовали. Но неясно, за что отвечает каждый класс, если выкинуть алгоритм Хаффмана. Например, было бы здорово сделать систему для чтения/записи битов, которая умеет писать/читать биты разных видов и считает статистику — это был бы понятный базовый класс. Но сейчас у вас деление больше по реализации, а не по смыслу. Архитектура не особо помогает понимать код, только копипаст снижает (что хорошо, но недостаточно).
  • Непонятно, зачем отдельный метод readLen()/writeLen(), когда на самом деле есть readUInt32_t()/writeUInt32_t().
  • Нехорошо, что всем наследникам надо руками поддерживать SizeInfo. Лучше это инкапсулировать в базовом AbstractIO. И сделать какие-то методы записи (возможно, побитовой, как общий знаменатель), всю запись пустить через них, а дальше считать статистику.
  • readDecoding()/writeEncoding — кажется, это отдельные методы для работы с объектом "кодировка Хаффмана", а не часть общего ввода-вывода.
• Huffman
  • HuffmanTree почему-то в конструкторе требует, чтобы деревья хранились в куче в unique_ptr. Это лишнее ограничение, нам это неважно. Тут требуется лишь забрать владение их корнями (что наглядно видно из реализации). Хватит rvalue-ссылки.
  • Конструктор от частот лучше сделать не конструктором а статической или свободной функцией, чтобы было пояснение "что он делает".
• HuffmanArchiver
  • Не стоит выводить на экран статистику автоматически. Пусть этим занимается main. Тогда весь ввод-вывод будет сосредоточен в нём. Сейчас вы, например, не можете протестировать корректность статистики.

Стиль 6/8

• namespace: лучше snake_case или CamelCase, не camelCase.
• Makefile:
  • Не хватает LDFLAGS и использования CXX для простой замены компилятора и стандартной библиотеки. Например, мне это надо на Ubuntu 16.04, чтобы скомпилировался namespace foo::bar: стандартный gcc не умеет.
  • Следует не руками прописывать зависимости, а автоматически через ключи -MMD -MP (при компиляции рядом с .o положится .d, который можно -include из Makefile; - нужен для отсутст)
  • Очень много дублирования кода. Для всех CPP можно сделать шаблон.
  • Можно подсмотреть сюда и проникнуться: ​http://make.mad-scientist.net/papers/advanced-auto-dependency-generation/. А ещё лучше — просто перейти на CMake.
• Свои заголовки надо включать с "", а не <>
• CLI
  • last_arg ыыкинуть, лучше i руками менять.
  • Из конструктора точно можно вынести генерацию args в отдельную функцию.
  • find() == end() --> count
  • f и file лучше склеивать прямо при парсинге, а не в конце проверять дублирование.
  • args[].size() проверять незачем: и так уже бывает исключение Arg duplicated. И это, опять же, лучше проверять в общем случае, а не для каждого отдельно.
• HuffmanIOProcessing
  • --> HuffmanIO
  • operator<< не должен сбрасывать буфер при помощи endl никогда.
  • decoding/encoding — не очень хорошие названия для таблицы кодирования.
  • reset()? Непонятно, что делает. rewindInput()?
  • Странно, что есть и isInputEmpty() и методы возвращают true/false. Лучше выбрать ровно один каноничный способ работы с потоком.
  • В деструкторе лучше не flush(), а проверить, что пользователь сделал flush(), имхо. Иначе у деструктора есть какие-то нетривиальные побочные эффекты.
• Huffman
  • Из HuffmanTree::getCharsEncoding можно выкинуть один if
  • Не используйте new в чистом виде никогда!
  • assert не на непустоту очереди, а на её размер.
  • Конструктор от одного символа — это что-то странное. Как минимум, он должен быть explicit. Да и конструктор от частот тоже (но его уже заменили на функцию).
  • Неясно, зачем дереву конструктор от двух деревьев, когда мы логически объединяем деревья только при построении из таблицы частот. Причём там на самом деле используются только вершины.
• HuffmanArchiver
  • getFrequencies — тут оно прямо пробегается и считает. Лучше calculate.
  • encode: незачем кодировать дважды и писать закодированную длину. Можно записать кое-что другое, будет так же красиво, но проще.
  • compress()/extract() — непонятно, зачем if в начале. Всегда кодируем, кроме каких-то случаев? Не понял. Заглушение ошибок? Хочу комментарий как минимум.
  • Лучше добавить assert'ы, что мы стоим в начале файла. Или [мыть тарелки перед едой](​http://blog.algoprog.ru/init-not-clear/).
  • В decode объявляйте переменные только там, где они реально нужны (bit), ни строчкой раньше.

[Egor Suvorov]

P.S. А ещё напоминаю про всякие final и noexcept.

[Surkov Petr]

Что-то исправил.
В момент переписывания архитектуры понял, что больше не хочу заниматься этой домашкой. Она осталась прежней.

[Egor Suvorov]

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

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

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

• В Huffman.hpp не хватает #include <string> (хотя std::string используется), из-за этого не компилируется под libc++:

  [ 31%] Building CXX object CMakeFiles/hw_03.dir/src/Huffman.cpp.o                                                       In file included from /home/yeputons/cpp19/surkov.petr/hw_03/src/Huffman.cpp:1:                                         In file included from /home/yeputons/cpp19/surkov.petr/hw_03/include/Huffman.hpp:3:                                     In file included from /usr/lib/llvm-10/bin/../include/c++/v1/unordered_map:409:                                         In file included from /usr/lib/llvm-10/bin/../include/c++/v1/__hash_table:15:                                           In file included from /usr/lib/llvm-10/bin/../include/c++/v1/memory:657:                                                /usr/lib/llvm-10/bin/../include/c++/v1/utility:306:9: error: implicit instantiation of undefined template 'std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >'                                                   _T2 second;                                                                                                                 ^                                                                                                               /home/yeputons/cpp19/surkov.petr/hw_03/src/Huffman.cpp:7:17: note: in instantiation of template class 'std::__1::pair<const unsigned char, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >' requested here                                                                                                                              return {{*c, ""}};
  

• На баллы не влияет: принимаются флаги -?file/-Xfile вместо --file почём зря.

Тесты 7.8/8

• Нет большого теста на какую-нибудь оптимальность кодов или что-то такое.

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

• Актуальны старые замечания про:
  • Битовые "строчки" в std::string
  • Необходимость руками считать статистику.
  • Непонятную зону ответственность HuffmanIO. Было бы лучше выделить: побитовую запись/чтение, потоки байт со статистикой, функции чтения-записи чисел, функции чтения-записи таблицы кодирования.
• CLI
  • Не очень плюсовый стиль: отдельно массив, отдельно количество аргументов. span<string_view>/vector<string_view>/vector<string> подошли бы больше.
• EncodingIO
  • Лучше в деструкторе не flush, а assert, что буфер сброшен.
• HuffmanTree
  • Неясно, зачем пустое дерево и публичный конструктор по умолчанию. Если для фабричных функций, то лучше его сделать приватным, а функции - друзьями. Или можно воспользоваться type_tags (смотри nullopt/in_place_t у std::optional, можно ещё у меня поинтересоваться).
  • Плохо понимаю разницу между HuffmanTree и HuffmanNode: конструкторы одинаковые, методы почти одинаковые... Кажется, что лучше в buildFromFreqTable манипулировать HuffmanNode и возвращать одно дерево. Тогда потребуется только конструктор от корня (возможно, от nullptr).
• Huffman
  • calculateFrequencies лучше отдельной свободной функцией и протестировать.

Стиль 6.5/8

• Не стоит хардкодить компилятор в CMakeLists.txt. Это надо делать при вызове: cmake . -DCMAKE_CXX_COMPILER=clang++-6.0. В CMakeLists.txt должно быть только что-то проектоспецифичное.
• НЕ ОБЪЯВЛЯЙТЕ ПЕРЕМЕННЫЕ ВНЕ ЦИКЛОВ (смотрю на TEST_CASE("Get bit") {)
• Все файлы должны завершаться \n (не incomplete line).
• CLI
  • input = args["f"][0], output = args["o"][0]; - тут лучше ; и на разных строчках.
  • std::string arg = std::string(argv[i]).substr(1 + static_cast<size_t>(len != 2)); - лучше убрать и называть везде аргументы как -a, -f, --file...
    • 1 + static_cast<size_t>(len != 2) - это жесть какая-то. Я несколько минут понимал, что это и зачем. Только потом понял, что это для длинных опций. Но вы тут тогда ещё и приняли -?file вместо --file, что нехорошо.
  • Вместо valPos лучше просто по i итерироваться.
  • В тестах вместо константы 6 лучше std::size(argv)
  • В тестах лишние фигурные скобки (Incompatible args и дальше).
• HuffmanIO
  • Никогда не надо делать flush/endl внутри operator<<.
  • Странно, что writeEncodingTable сбрасывает статистику по дополнительным символам.
  • Тесты
    • Вместо (1 << 7) + (1 << 6) + (1 << 5) + (1 << 4) + (1 << 2) + 1 лучше 0b1110'0101
      • Вместо string s = s1 + s2 лучше string s = "11110101" "10011001" (подряд идущие строковые литералы препроцессор соединяет). Ну или хотя бы без промежуточных переменных: string s = string("foo") + "bar");
• Huffman
  • HuffmanTree::HuffmanNode::getCharsEncodingTable - можно упростить: сразу создайте возвращаемое значение, а потом range-based-for сразу по результату функции без промежуточной переменной. Так можно: ​https://stackoverflow.com/a/9657748/767632
  • Тесты
    • В одном месте не заиспользовали свой FreqTable
• HuffmanArchiver
  • Тесты
    • Лишние пустые строчки в начале TEST_SUITE
      • uniform_int_distribution<>dis - не хватает пробела перед dis
      • info1/info2 - не очень понятные имена. Равно как и ss1/2/3, лучше описать, что именно в них лежит.
• main
  • Многовато инклудов

[Egor Suvorov]

Кстати, решение прям классное. Серьёзное по архитектуре - я путаюсь с EncodingIO/DecodingIO плюс std::string. Это исправить, будет 4.8/5 по архитектуре.