HW #1

[Казаков Никита]

можно добавить assert(argc != 0);
чтобы компилятор не ругался на неиспользованную переменную

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

Решение не собирается, отсутствует сткруктура папок и разделение исходного кода на файлы-модули:

{hw_01}[2071]$ pwd && svn up && svn status
/home/hfx/dvl/cpp19/kazakov.nikita/hw_01
Updating '.':
At revision 1902.
{hw_01}[2072]$ make
make: /snap/clion/92/bin/cmake/linux/bin/cmake: Command not found
Makefile:176: recipe for target 'cmake_check_build_system' failed
make: *** [cmake_check_build_system] Error 127
{hw_01}[2073]$ which cmake
/usr/bin/cmake
{hw_01}[2074]$ ls
CMakeLists.txt  hw_1.c  lena_007.bmp  lena_512.bmp  Makefile

Ниже комментарии по стилю/логике работы программы (hw_1.c):
15: сложно придумать более общее имя для функции. Думаю, вы имели в виду что-то вроде pixel_offsеt. Если так, странно, что аргументы знакового типа, а не size_t;
19: комментарий вводит в заблуждение: а что если передадут не-фотку? Fix: s/исходную фотку/изображение в формате BMP
21: загружается пиксель, а не bmp (имя функции не соотвествует тому, что она делает); в bmp пиксели идут в BGR порядке, код считывает как RGB; считать можно за один вызов fread (код будет более компактным);
37: копировать поэлементно явно не нужно. Структуры можно присваивать (поэлементное копирование произойдет неявно);
59: имя файла надо брать из argv; argc можете использовать для проверки ожидаемого количества аргументов; имя переменной: s/some_file/input_file; не проверяется удалось ли открыть файл;
63: лучше считывать структуру заголовка целиком (и проверять успешно ли прошло считывание); с hardcoded смещениями сложно работать в долгосрочной перспективе;
105: *((int *)(tilt + 18)) = cut_width, но лучше обновлять поля структуры, описывающей заголовок, и записывать его в файл.

Баллы: корректность - 0, стиль - 2 (т.к. код существенно изменится, полагаю, и неясно работает ли текущая версия).

[Казаков Никита]

На всякий случай закоммитил вместе с CMake'ом. Пока нормально вырезает только квадраты любого размера. Поменял название некоторых функций. Добавил пару assert'ов.

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

Решение все еще не собирается:

{hw_01}[2128]$ pwd && svn up && svn status
/home/hfx/dvl/cpp19/kazakov.nikita/hw_01
Updating '.':
At revision 2019.
{hw_01}[2129]$ make
CMake Error: The source directory "/home/just/cpp2019/kazakov.nikita/hw_01" does not exist.
Specify --help for usage, or press the help button on the CMake GUI.
Makefile:206: recipe for target 'cmake_check_build_system' failed
make: *** [cmake_check_build_system] Error 1
{hw_01}[2130]$ mkdir build
{hw_01}[2131]$ cd build/
{build}[2132]$ cmake ..
CMake Error: The current CMakeCache.txt directory /home/hfx/dvl/cpp19/kazakov.nikita/hw_01/CMakeCache.txt is different than the directory /home/just/cpp2019/kazakov.nikita/hw_01 where CMakeCache.txt was created. This may result in binaries being created in the wrong place. If you are not sure, reedit the CMakeCache.txt

Напишите Makefile, так будет надежнее. Также в папке с решением не должно быть лишних файлов.

Глобальные замечания:

• считывайте/записывайте заголовки/пиксели за один вызов fread (для этого надо завести структуру с полями как у заголовка и указать, что она хранится в памяти без выравнивания);
• проверяйте результат, который возвращают функции ввода/вывода;
• load_bmp должен считывать весь файл, а не один пиксель, crop, rotate - модифицировать все изображение, а не отдельный пиксель.

Также я не увидел, где проверяется ожидаемое количество аргументов (видел только проверку, что их не ноль, которая бессмысленна, так как у программы всегда есть минимум один аргумент - имя).

Чините сборку, баллы пока остаются без изменений. Если удастся починить до воскресенья, проверю решение еще раз как вторую попытку сдачи.

[Казаков Никита]

Сделал просто Makefile. Если и так не будет собираться, то я не знаю, что делать. У меня собирается и работает. В интернете по этому поводу ничего не нашел.

Со считыванием в один вызов пока не разобрался. Также не понял, что значит проверка результата ввода и вывода. Проверка на то, что массив заполнен?
load_bmp, crop, rotate исправил.
Проверку на количество элементов с помощью assert сделал.

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

Сделал просто Makefile. Если и так не будет собираться, то я не знаю, что делать. У меня собирается и работает. В интернете по этому поводу ничего не нашел.

“У меня собирается и работает” значит “я попробовал собрать решение на чистой системе и оно собралось”? Как бы то ни было, написание Makefile’a решило проблему.

Со считыванием в один вызов пока не разобрался.

Вот пример, чтобы вам попроще было:

#pragma pack(push, 1) // запрет выравнивания для структур определенных после
struct Foo {
  uint32_t bar;
  uint16_t baz;
};
#pragma pack(pop) // восстановление предыдущей политики выравнивания

struct Foo foo;
assert(fread(&foo, sizeof foo, 1, input_file) == 1);

Также не понял, что значит проверка результата ввода и вывода. Проверка на то, что массив заполнен?

Проверка значения, которое возвращает функция fread/fwrite. Если оно не равно числу элементов, то произошла какая-то ошибка (см. man fread) и нужно как-то ее обработать (достаточно assert’а). To же самое касается fopen, fseek.

Корректность

Не работает на прямоугольных изображениях (valgrind находит ошибки, сгенерированные файлы визуально отличаются от ожидаемых). Не увидел, где в коде обрабатывается выравнивание при чтении.
На квадратных изображениях результат визуально не отличается от ожидаемого, но отличается бинарно. Пример работы теста “повернуть изображения целиком 4 раза и сравнить с оригиналом”:

{hw_01}[2182]$ pwd && svn status && svn up
/home/hfx/dvl/cpp19/kazakov.nikita/hw_01
Updating '.':
At revision 2037.
{hw_01}[2183]$ make
mkdir obj
gcc src/bmp.c -c -Iinclude -o obj/bmp.o -Wall -Wextra -Werror -g
gcc src/main.c -c -Iinclude -o obj/main.o -Wall -Wextra -Werror -g
gcc -o hw_01 obj/main.o obj/bmp.o -Wall -Wextra -Werror -g
{hw_01}[2184]$ ./hw_01 crop-rotate samples/lena_512.bmp out_lena1.bmp 0 0 512 512
{hw_01}[2185]$ ./hw_01 crop-rotate out_lena1.bmp out_lena2.bmp 0 0 512 512
{hw_01}[2186]$ ./hw_01 crop-rotate out_lena2.bmp out_lena3.bmp 0 0 512 512
{hw_01}[2187]$ ./hw_01 crop-rotate out_lena3.bmp out_lena4.bmp 0 0 512 512
{hw_01}[2188]$ diff samples/lena_512.bmp out_lena4.bmp 
Binary files samples/lena_512.bmp and out_lena4.bmp differ

Стиль

Часть замечаний в комментариях выше все еще актуальна.
Общее замечание - перенесите функции загрузки, сохранения, модификации файла в src/bmp.c. В текущей версии там находятся лишь код копирования пикселей. Пример для load_bmp:

int load_bmp(FILE *bmp_file, struct BmpImage *image /*хранит внутреннее представление изображения*/) {
  // считать BMP header, разобрать его, обновить поля image
  // считать DIP header, разобрать его, обновить поля image
  // считать изображение, обновить/инициализировать поля image
}

main.c:44. Старайтесь не использовать VLA, выделяйте массивы в динамической памяти, тем более под все изображение;
main.c:91. Что если 1 < addition? Технически будут записаны данные не из zero, а это ошибка.

Баллы: корректность 2, стиль 3.

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

Корректность

• не обновляются размер файла и изображения в заголовках генерируемого файла;
• при работе с прямоугольными изображениями valgrind находит ошибки (main.c:51. Попытка записать из однобайтового буфера (bmp.h:17) 2 или 3 байта в случае смещения такого размера);
• bmp.c:46. размер int по стандарту 2+ байта, padding может быть равен 3 -> переполнение буфера на платформах с sizeof int === 2.

Стиль

include/bmp.h

5: uint8_t используется, а stdint.h не подключен;
20: аналогично для FILE*.

src/main.c

42: запись в файл нужно было вынести в функцию save_bmp (по условию);
42: что такое 54?
51: дублирование нетривиальной логики (подсчет padding’а).

src/bmp.c

7, 13: если определили структуры в bmp.h, то нужно подключить его и пользоваться определениями оттуда. Определения struct BMP_image не совпадают (тип padding);
27: текущий вариант чтения/записи полей заголовков тяжело поддерживать. Нужно было сделать что-то вроде такого:

#pragma pack(push, 1)
struct BmpHeader {
  uint16_t magic;
  /* прочие поля */
};
struct DibHeader {
  /* поля DIB-заголовка */
};

struct DibHeader dib_header;
assert(fread(&dib_header, sizeof dib_header, 1, in_file) == 1);
...
dib_header.width = new_width;
...
assert(fwrite(&dib_header, sizeof dib_header, 1, out_file) == 1);

37: последний параметр fseek нужно задавать константой (SEEK_SET, например);
57: это не итератор, а скорее индекс (имя переменной запутывает);
77: можно было написать так: *((int *)rolled_image->header_field + 18) = cropped_image->height; вместо копирования по одному байту, а еще лучше определить структуры заголовков явно и писать в их поля (см. пример выше).

Баллы: корректность 14, стиль 4.5.

Если неясно как все-таки можно было читать заголовки, используя структуры, напишите письмо с вопросом/уточните на практическом занятии.