LINUX.ORG.RU

Краткая история о том, как не надо писать (и улучшать) код

 , ,


1

1

По мотивам разговора в Почему программы на с++ тормозят :) решил оформить это отдельной темой.

В исходниках GNU grep я наткнулся на такое:

/* Whether to output filenames.  1 means yes, 0 means no, and -1 means
   'grep -r PATTERN FILE' was used and it is not known yet whether
   FILE is a directory (which means yes) or not (which means no).  */
static int out_file;
  /* Which command-line options have been specified for filename output.
     -1 for -h, 1 for -H, 0 for neither.  */
  int filename_option = 0;
  /* Don't output file names if invoked as 'grep -r PATTERN NONDIRECTORY'.  */
  if (out_file < 0)
    out_file = !!S_ISDIR (st.st_mode);

Тут явно напрашивается enum вместо магических констант. О чем я сразу подумал. А дальше в коде я наткнулся еще на такой однострочник, с которым без поллитры не разобраться:

  out_file = (filename_option == 0 && num_operands <= 1
              ? - (directories == RECURSE_DIRECTORIES)
              : 0 <= filename_option);

В общем, я подумал, что эта лапша наверное родом откуда-то из конца 80-х - начала 90-х, когда код писали в «хакерских традициях». Но нет. Всё это безобразие было введено в 2019-м году в 3-х коммитах под следующими заголовками:

Автор сначала ввёл enum-ы как разумный разработчик. А потом выкинул их, заменив на числа и магические операции сравнения а ля больше/меньше/равно нулю.

Не делайте так. Не хакерствуйте над битами как для конкурса IOCCC.

★★

Последнее исправление: wandrien (всего исправлений: 2)
Ответ на: комментарий от LINUX-ORG-RU

А Фабриса Беллара приводить как плохой пример, ну такое себе, он родил в одно рыло ffmpeg,qemu,tcc,QuickJS и ничё. Всем бы так.

Почему плохой пример? Это хороший пример во всех смыслах. Он писал OTCC на конкурс.

А продуктивность Фабриса Белларда космическая. Таких программистов наверное единицы в мире.

wandrien ★★
() автор топика
Ответ на: комментарий от thesis

Например, во втором коммите он уже сам вводит:

+  /* Command-line options for filename output.  */
+  enum
+  {
+   NO_FILENAME = -1,	/* --no-filename (-h) */
+   FILENAME_DEFAULT,	/* Neither option is specified.  */
+   WITH_FILENAME,	/* --with-filename (-H) */
+  } filename_option = FILENAME_DEFAULT;

А в третьем - «упрощает».

P.S. Я тут щас гибкий многострочный grep пишу как программу на awk. На awk это делать на порядок проще, чем на Си. Выложу на github, если допишу.

wandrien ★★
() автор топика
Ответ на: комментарий от wandrien

То есть вместо

 - (directories == RECURSE_DIRECTORIES)

ты предлагаешь

 (directories == RECURSE_DIRECTORIES?NO_FILENAME:FILENAME_DEFAULT)

Зато с enum :\

Psilocybe ★★★★
()
Последнее исправление: Psilocybe (всего исправлений: 1)
Ответ на: комментарий от wandrien

Вот поэтому я всегда делаю такие enum на символах/строках, а не на числах. Чтобы потом какой-нибудь дурачок не стал делать арифметику и сравнения с этим числами.

no-such-file ★★★★★
()
Ответ на: комментарий от wandrien

Глядя на имена констант не ясно к чему они относятся к filename_option или к out_file, поэтому enum этот недоделаный. А возможно у него возникли проблемы с тем, что grep не собирается на какой-то платформе с древним компилятором не поддерживающим enum для C

cobold ★★★★★
()
Последнее исправление: cobold (всего исправлений: 1)
Ответ на: комментарий от cobold

grep не собирается на какой-то платформе с древним компилятором не поддерживающим enum для C

Можно долго гадать, но я заметил, что у проекта GNU вообще проблемы с тем, чтобы позиционировать себя сейчас - кто они и про что они. Одновременно присутствует и поддержка кучи древних платформ, и дурацкие мувы типа объявления egrep устаревшей (что создало бесполезную нагрузку по механической замене текста для мейтенеров сотен, если не тысяч проектов).

На организационном уровне код проектов переусложнён обвязками для поддержки редких конфигураций (все эти libiberty, gnulib и т.п.), а на уровне кода отдельных модулей зачастую имеет не очень высокое качество и страдает отсутствуем внутренней гибкости и лишней связность компонентов.

wandrien ★★
() автор топика

Всё нормально с этими константами. -1 - принудительное выключение, +1 - принудительное включение, 0 - дефолт. Подписывать их какими-то названиями явно лишнее, это только визуально (и логически!) замусорит код.

 out_file = (filename_option == 0 && num_operands <= 1
              ? - (directories == RECURSE_DIRECTORIES)
              : 0 <= filename_option);

А тут согласен не очень красиво. И не очень даже понятно что тут хотели сделать. Наверно там дальше out_file ещё как-то изменяется.

firkax ★★★★★
()
Последнее исправление: firkax (всего исправлений: 2)

Замена констант на энумерацию может производиться только если они совсем одинаковы по смыслу. Например цвет светофор.КРАСНЫЙ и подобное, ибо тут нельзя по другому. Если промахнуться с логикой энумерации можно получит -1000 в карму от коллег. Ладно ещё свой собственный код, а если сопровождают другие надо быть осмотрительным.

monkdt
()

Гнутые утилиты – это рак и червиё. Давай для начала поговорим о том, что гнутый /bin/true может выдать 1 в некоторых случаях.

Код здесь: https://github.com/coreutils/coreutils/blob/master/src/true.c

hateyoufeel ★★★★★
()
Последнее исправление: hateyoufeel (всего исправлений: 1)
Ответ на: комментарий от firkax

А ничего, что у двух переменных там даже семантика этих констант разная? У одной дефолт это 0, а у другой -1. Вот так вот внимательно ты почитал код и попал точно в ловушку, заложенную разрабом.

Счастливого рефакторинга.

wandrien ★★
() автор топика
Ответ на: комментарий от wandrien

Я про filename_option писал. Это переменная с описанием состояния присланных юзером опций. Таких опций (вкл/выкл/не указано) может быть много разных, под каждую делать однотипные (но разные) буквенные константы не нужно. Возможно, можно было сделать FORCE_ENABLE=1, FORCE_DISABLE=-1, UNSPECIFIED=0 (не уточняя конкретную опцию), но это уже дело вкуса.

попал точно в ловушку, заложенную разрабом

Нет, не попал.

С out_file у них проблема, да, вариант «дефолт» (и значение -1) там неоправдан совершенно, нужно было проверять всё в одном месте, тогда и странная формула из цитаты выглядела бы намного нагляднее.

firkax ★★★★★
()
Последнее исправление: firkax (всего исправлений: 2)
Ответ на: комментарий от firkax

Ну что касается подхода в целом, то если кто-то считает, что этот код более соответствует духу хорошего кода

  /* Don't output file names if invoked as 'grep -r PATTERN NONDIRECTORY'.  */
  if (out_file < 0)
    out_file = !!S_ISDIR (st.st_mode);

чем код в таком стиле:

  /* Don't output file names if invoked as 'grep -r PATTERN NONDIRECTORY'.  */
  if (print_file_names == PRINT_FILE_NAMES_AUTO)
    print_file_names = S_ISDIR (st.st_mode) ?
                       PRINT_FILE_NAMES_ENABLED :
                       PRINT_FILE_NAMES_DISABLED;

то с таким разрабом мне будет сложно сработаться.

Тут рефакторингу подлежит всё, начиная от имён переменных.

Задача кода — выражать логику программы на языке, наиболее близком к предметной области. Поскольку код в первую очередь предназначен для чтения людьми.

wandrien ★★
() автор топика
Ответ на: комментарий от firkax

-1 - принудительное выключение, +1 - принудительное включение, 0 - дефолт

Завтра тебе понадобится второй дефолт, который используется в определённых условиях и который надо отличать от принудительного включения. И вся твоя красивая концепция пойдёт по варенику.

no-such-file ★★★★★
()
Ответ на: комментарий от wandrien

Оба варианта плохие. В твоём куча тавтологии.

Как я понял, проверку на директорию поставить рядом с проверкой опции не получается, это разные блоки кода, поэтому нормальный вариант получается как-то так:

Вместо первой сложной формулы это:

print_file_names = (filename_option?filename_option:(num_operands>1));
Вместо «хакерской» формулы для директорий это:
if(!print_file_names) print_file_names = (recurse && S_ISDIR(st.st_mode)) ? 1 : -1;
(кстати компилятор сам её оптимизирует в такое же хакерство на асме скорее всего)

Ну и в обоих переменных 0 = неопределено.

Хороший короткий и интуитивно понятный код. Без неочевидных построений как у них и без тавтологических простыней как у тебя.

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

if(!print_file_names) print_file_names = 1;

firkax ★★★★★
()
Последнее исправление: firkax (всего исправлений: 5)
Ответ на: комментарий от no-such-file

Нет никакого второго дефолта. Это переменная с запросом от юзера. Дефолт означает что юзер эту опцию не уточнил.

красивая концепция

Но хоть это ты признал.

firkax ★★★★★
()
Последнее исправление: firkax (всего исправлений: 1)
Ответ на: комментарий от firkax

Нет никакого второго дефолта

Сегодня нет, завтра есть.

Дефолт означает что юзер эту опцию не уточнил

Другой дефолт может быть при других опциях. Самое очевидное: дефолт поменяли, но оставили опцию чтобы было «как раньше».

no-such-file ★★★★★
()
Ответ на: комментарий от firkax

Не важно какой там дефолт, важно что это дефолт.

Конечно важно, это же дефолтное поведение, а не просто значение. Т.е. исполняться должен разный код.

хоть это ты признал

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

no-such-file ★★★★★
()
Ответ на: комментарий от no-such-file

Конечно важно, это же дефолтное поведение, а не просто значение. Т.е. исполняться должен разный код.

В контексте парсера опций - не важно. Ну а дальнейший код, конечно, должен знать чему этот дефолт соответствует. Если дефолт статический, то ноль можно заменить на явное дефолтное ненулевое значение сразу после парсера (но не до, иначе парсеру сложнее дублирующиеся опции будет ловить). Если динамический (т.е. зависит от run-time обстоятельств) - то его должен учитывать дальнейший код.

firkax ★★★★★
()
Ответ на: комментарий от hateyoufeel

гнутый /bin/true может выдать 1 в некоторых случаях

Не может, и это очевидно для любого, умеющего читать сишку. Этот код реализует одновременно true и false, в зависимости от макроса EXIT_STATUS.

annulen ★★★★★
()
Ответ на: комментарий от annulen

Нет, там 1 при ошибке вывода в stdout возвращается. То есть /bin/true --help (например: /bin/true --help > /dev/nvidiactl или найди какое-нить устройство в dev которое явно не принимает данные просто так но имеет доступ на запись) вернёт 1 если не сможет ответ напечатать. Думаю скриптов это не страшно, но такое количество побочной логики всё равно смущает.

firkax ★★★★★
()
Последнее исправление: firkax (всего исправлений: 1)
Ответ на: комментарий от cobold

какой-то платформе с древним компилятором не поддерживающим enum для C

Даже ack под миниксом их поддерживает (и более-менее совместим с ANSI C). И это при том, что греп скорее всего не соберётся из-за лимита в 64 кБ на данные и стек.

luke ★★★★★
()
Ответ на: комментарий от annulen

Там не SIGHUP, там другая ошибка (в данном случае EINVAL), которую он вручную из write() где-то в глубине glibc получает и потом при выходе видит что stdout с ошибкой. Но в целом да, суть похожа.

$ LANG= /bin/true --version >> /dev/nvidiactl || echo bad
/bin/true: write error: Invalid argument
bad
firkax ★★★★★
()
Ответ на: комментарий от luke

У программы /bin/true есть ровно одна задача: вернуть 0. Всё остальное — гнутое переусложнение. Сравни гнутый код с кодом из OpenBSD, для примера: https://github.com/openbsd/src/blob/master/usr.bin/true/true.c

hateyoufeel ★★★★★
()
Ответ на: комментарий от drl

Код должен быть понятным без комментариев. Исключение составляют случаи, когда нужно описать какие-то нетривиальные кейсы. Как вот в этом случае, имеет смысл указать, что требуется эта самая «автоматическая» логика, так как это требует мануал на программу. То есть пишем, «согласно с чем» это. А не «как это работает».

И второе, код должен быть пригоден для тестирования. По этому критерию, с файлом grep.c там тоже всё не очень хорошо.

wandrien ★★
() автор топика
Ответ на: комментарий от cobold

Нихрена они уже толком не делают. Сейчас в других областях и проектах толковые разрабы.

А качество кода GNU всегда было так себе. При чем как на архитектурном уровне, так и на уровне отдельных модулей.

Хак на хаке, которые годами висят наследием.

wandrien ★★
() автор топика
Ответ на: комментарий от wandrien

Нет каких-то абстрактных «разработчиков GNU» в вакууме, над каждым проектом работают разные люди. Над проектами вроде gcc и glibc работают профессионалы на зарплате у корпораций. У нишевых проектов вроде mc свои сообщества. А над coreutils, по-видимому, работают интересные личности вроде сабжа.

annulen ★★★★★
()
Ответ на: комментарий от annulen

Бородатые ГНУ-хацкеры даже не поленились задокументировать.

info true

...
   Note, however, that it is possible to cause ‘true’ to exit with
nonzero status: with the ‘--help’ or ‘--version’ option, and with
standard output already closed or redirected to a file that evokes an
I/O error.  For example, using a Bourne-compatible shell:

     $ ./true --version >&-
     ./true: write error: Bad file number
     $ ./true --version > /dev/full
     ./true: write error: No space left on device
...
undef ★★★
()
Ответ на: комментарий от firkax

Думаю скриптов это не страшно, но такое количество побочной логики всё равно смущает.

В /bin/true это не страшно, но в других утилитах такой подход приводил в том числе и к дырам, потому что переменные для контроля локалей прокидываются из окружения текущего юзера. И если у тебя бинарник с suid, это может привести к некоторым лулзам и local privilege escalation.

hateyoufeel ★★★★★
()
Ответ на: комментарий от firkax

Там всё лишнее. cp может выжрать всю память и сдохнуть, например, потому что вместо рекурсивного прохода по директориям он строит хэш-таблицу и идёт по ней. Гнутые тулзы – отличный пример того, как делать НЕ НАДО.

https://lists.gnu.org/archive/html/coreutils/2014-08/msg00012.html

hateyoufeel ★★★★★
()