WW #4

[Sokolov Viacheslav]

Не хватает зависимостей .o от .h

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

copy в mergesort.h же не должно быть?

Мне кажется,

 84 const type_t types[] = {{.name = "int",
 85                          .size = sizeof(int),
 86                          .comparator = comparator_int,
 87                          .parser = parser_int,
 88                          .printer = printer_int},
 89                         {.name = "char",
 90                          .size = sizeof(char),
 91                          .comparator = comparator_char,
 92                          .parser = parser_char,
 93                          .printer = printer_char},
 94                         {.name = "str",
 95                          .size = sizeof(char *),
 96                          .comparator = comparator_string,
 97                          .parser = parser_string,
 98                          .printer = printer_string}};

менее читаемо, чем

 84 const type_t types[] = {
 85     {
 86         .name = "int",
 87         .size = sizeof(int),
 88         .comparator = comparator_int,
 89         .parser = parser_int,
 90         .printer = printer_int
 91     },
 92     {
 93         .name = "char",
 94         .size = sizeof(char),
 95         .comparator = comparator_char,
 96         .parser = parser_char,
 97         .printer = printer_char
 98     },
 99     {
100         .name = "str",
101         .size = sizeof(char *),
102         .comparator = comparator_string,
103         .parser = parser_string,
104         .printer = printer_string
105     }
106 };

Не настаиваю на конкретном способе выранивания, хочу лишь зафиксировать проблему с выравниванием "по первой скобке": часть кода "уезжает" направо, строчки становятся длинными (но с небольшим количеством непробельных символов); совершенно не сочетается с рекомендацией держать ширину строк не больше скольки-то. В стандартной библиотеке c++ как раз наглядно видна проблема.

parser_int можно существенно упростить:

 21   int a = atoi(s);
 22   copy(&a, dist, element_size);

та же история с char и str.

Обращаю внимание на const: много где не хватает

58 void printer_int(CONST void *data, size_t elements) {
59   CONST int *cur = data;

 4 void copy(CONST void *source, void *dist, size_t element_size) {

 15 int comparator_string(const void *a, const void *b) {
 16   assert(a != NULL && b != NULL);
 17   return strcmp(*(CONST char **)a, *(CONST char **)b);
 18 }  

и тд

Еще рекомендация: использовать NULL для нулевого указателя.

Это все привычки, которые будут полезны далее, в C++.

main.c
24 data = malloc(elements * element_size);
а если не удалось?

mergesort.c
если первый рекурсивный запуск не удался, второй делать незачем. Для этого удобно использовать short-circuit evaluation: ​https://en.cppreference.com/w/c/language/operator_logical

Вместо нескольких циклов можно сделать один, в котором предполагается, что один из указателей уже достиг границы. Это позволит сократить объем (копирования) кода.
Можно проверить большее количество контрактов (element_size != 0; element_size * elements не переполняется; в конечном итоге массив отсортирован).

Именование: кажется, bound соответсвует лучше, чем limit.

[Brilliantov Kirill]

Насчет форматирования, просто я пользуюсь палгином для vim, который выравнивает код и я так сильно привык, что мне очень не хочется его отключать... Ну он вроде все остальное хорошо выравнивает

[Sokolov Viacheslav]

Какой плагин?

[Brilliantov Kirill]

Replying to Sokolov Viacheslav:

Какой плагин?

Plug 'google/vim-codefmt'
Plug 'google/vim-maktaba'
Plug 'google/vim-glaive'

[Sokolov Viacheslav]

Replying to Brilliantov Kirill:

Replying to Sokolov Viacheslav:

Какой плагин?

Plug 'google/vim-codefmt'
Plug 'google/vim-maktaba'
Plug 'google/vim-glaive'

Под капотом clang-format, который можно настроить (при желании) ​https://clang.llvm.org/docs/ClangFormatStyleOptions.html

[Brilliantov Kirill]

Replying to Sokolov Viacheslav:

Replying to Brilliantov Kirill:

Replying to Sokolov Viacheslav:

Какой плагин?

Plug 'google/vim-codefmt'
Plug 'google/vim-maktaba'
Plug 'google/vim-glaive'

Под капотом clang-format, который можно настроить (при желании) ​https://clang.llvm.org/docs/ClangFormatStyleOptions.html

Хорошо, постараюсь найти время и сделать это. Спасибо)

[Sokolov Viacheslav]

Все еще теряется константность. На допбалл надо поправить.

mergesort.c

39 } else {
40 break;
41 }

не нужен, никогда не выполняется.

sorted не будет работать на 0 элементов. Лучше сразу ставить assert-ы, чтобы не думать потом, когда вдруг поменяется предположение вызывающей функции.

 18   int res1 = 0, res2 = 0;
 19   res1 = mergesort(array, mid, element_size, comparator);                                            
 20   if (res1 != -1)                                                                                    
 21     res2 = mergesort(left_bound, elements - mid, element_size, comparator);                          
 22   if (res1 == -1 || res2 == -1)
 23     return -1;
 24   char *buffer = (char *)malloc(element_size * elements);
 25   if (buffer == NULL) {
 26     return -1;
 27   }

переписывается как

    int res = 0;
    res = res || mergesort(array, mid, element_size, comparator);                                            
    res = res || mergesort(left_bound, elements - mid, element_size, comparator);                          
    char *buffer = res ? NULL : (char *)malloc(element_size * elements);
    if (buffer == NULL) {
      return -1;
    }

element_size * elements используется несколько раз, лучше сохранить в отдельную переменную. Кстати, а что, если это выражение переполняется?

44 cur -= element_size * elements;

можно же cur = buffer;
btw, имена не самые удачные

 45   size_t i = 0;
 46   while (i != elements) {
 47     copy(cur, array, element_size);
 48     cur += element_size, array += element_size;
 49     i++;
 50   }

типичный цикл for

51 array -= elements * element_size;
эта операция нужна только для assert-а, почему бы не занести внутрь вызова? (просто -, а не -=); кроме того, вариант с запоминанием исходного состояния более читаемый.

чем
my_memory.c

10     destination++, cur++;

лучше, чем

destination++;
cur++;

?

что еще можно сделать лучше:
разбить type.h и type.c на несколько файлов (по файлу на тип и один файл, их объединяющий) - это позволит поддерживать новые типы не трогая уже имеющиеся файлы (точнее модифицируя единственное место, где все они включены); вся работа с одним типом находится в одном месте (рядом), труднее ошибиться

добавить перевод строки

28 printf("Error: memory allocation failed\n");

27 if (res < 0) {

здесь нужен assert(res == -1). Если допустимы другие возращаемые значения, например, -2, то программа перестает быть корректной.