Это злоупотребление макросами?

Ответ 1

Потому что это не то же самое. Если вы добавите то же самое в функцию, у вас будет короткая функция, которая возвращает DCD_BADWRITE, если X равно -1. Макрос, о котором идет речь, однако, возвращается из содержащей функции. Пример:

int foobar(int value) {
    CHECK_FREAD(value, "Whatever");
    return 0;
}

будет расширяться до:

int foobar(int value) {
    if (value == -1) {
        return (DCD_BADWRITE);
    };
    return 0;
}

Если бы это была функция, внешняя функция (foobar) с удовольствием проигнорировала бы результат и вернула бы 0.

Макрос, который выглядит так же, как функция, но ведет себя по-разному решающим образом, является серьезной ИМО без ИО. Я бы сказал, да, это злоупотребление макросами.

Ответ 2

Это не может быть сделано как функция, потому что return возвращается из вызывающей функции, а не из блока внутри макроса.

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

Ответ 3

Я нахожусь на заботе о том, следует ли называть такой макрос * abuse *, но я скажу, что это не очень хорошая идея.

Я бы сказал, что в общем случае вам следует избегать использования return в макросе. Трудно создать макрос, который правильно обрабатывает возврат, не создавая поток управления функции, который вызывает это неудобно или трудно понять. Вы определенно не хотите "случайно" возвращаться из функции, не осознавая этого. Это может раздражать отладки.

Кроме того, этот макрос может вызвать проблемы с потоком кода в зависимости от того, как он используется. Например, код

if (some_condition)
    CHECK_FREAD(val, "string")
else
    something_else();

не будет делать то, что вы могли бы подумать (else становится связанным с if внутри макроса). Макрос должен быть заключен в { }, чтобы убедиться, что он не изменяет никаких внешних условий.

Кроме того, второй аргумент не используется. Очевидной проблемой здесь является реализация, не соответствующая документации. Скрытая проблема заключается в том, что макрос вызывается следующим образом:

char* messages[4];          // Array of output messages
char* pmsg = &messages[0];
....
CHECK_FREAD(val, pmsg++);

Вы ожидаете, что указатель переместится к следующему элементу массива после вызова макроса. Однако второй параметр не используется, поэтому приращение никогда не происходит. Это еще одна причина, по которой утверждения с побочными эффектами вызывают проблемы в макросах.

Вместо использования макроса для выполнения всего этого, почему бы не написать функцию для инкапсуляции read и проверки ошибок? Он может возвращать ноль по успеху или код ошибки. Если код обработки ошибок является общим для всех блоков read, вы можете сделать что-то вроде этого:

int your_function(whatever) {
    int ret;
    ...

    if ((ret=read_helper(fd, &input_integer)) != 0)
        goto error_case;

    ...

    // Normal return from the function
    return 0;

error_case:
    print_error_message_for_code(ret);
    return ret;
}

Пока все ваши вызовы read_helper присваивают их возвращаемое значение ret, вы можете повторно использовать тот же блок кода обработки ошибок во всей функции. Ваша функция print_error_message_for_code просто примет код ошибки в качестве ввода и использует switch или массив, чтобы распечатать соответствующее ему сообщение об ошибке.

Я признаю, что многие люди боятся goto, но повторное использование общего кода обработки ошибок вместо серии вложенных блоков и переменных условий может быть подходящим для него вариантом использования. Просто сохраните его в чистоте и читабельности (по одной метке на каждую функцию и просто сохраните код обработки ошибок).

Ответ 4

Является ли это злоупотреблением вопросом вкуса. Но я вижу некоторые основные проблемы с ним

• документация полностью неправильно
• реализация очень опасна из-за if и возможного проблема обмана else
• Именование не означает, что это является предварительным возвратом окружающая функция

так что это окончательно очень плохой стиль.

Ответ 5

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

char *buf = malloc(1000);
if (buf == 0) return ENOMEM;

ret_val = read(fd, buf, 1000);
CHECK_READ(ret_val, "buffer read failed")

// do something with buf

free(buf);

Если я верю в документацию, у меня нет оснований подозревать, что этот код течет в памяти всякий раз, когда сбой чтения. Даже если документация была правильной, я предпочитаю, чтобы поток управления в C был явно очевидным, а это значит, что не скрыты макросами. Я также предпочел бы, чтобы мой код выглядел нечетко согласованным между случаями, когда у меня есть ресурсы для очистки, а также случаи, когда я этого не делаю, вместо того, чтобы использовать макрос для одного, а не для другого. Таким образом, макрос не по моему вкусу, даже если он не является окончательно оскорбительным.

Чтобы сделать то, что говорит документация, я бы предпочел написать что-то вроде:

check(retval != -1, "buffer read failed");

с check является функцией. Или используйте assert. Конечно, утверждение только делает что-либо, если NDEBUG не задан, поэтому он не подходит для обработки ошибок, если вы планируете отдельные сборки для отладки и выпуска.

Чтобы сделать то, что на самом деле делает макрос, я бы предпочел полностью пропустить макрос и написать:

if (retval == -1) return DCD_BADREAD;

Это вряд ли длинная строка кода, и нет никакой реальной выгоды в распространении разных экземпляров. Также, вероятно, не стоит думать, что вы улучшите ситуацию, инкапсулируя/скрывая существующие, хорошо документированные средства, с помощью которых read указывает ошибки, которые широко понимаются программистами C. Если ничего другого, эта проверка полностью игнорирует тот факт, что read может производить меньше данных, чем вы просили, по какой-либо заметной причине. Это не ошибка макроса напрямую, но вся идея макроса, похоже, исходит из плохой проверки ошибок.

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

Макросы, которые управляют потоком, могут быть не злоупотребляющими, но я думаю только в ситуациях, когда они выглядят как поток управления и точно документируются. Simon Tatham - совлокальные макросы spring - некоторые люди не любят совлокальные подпрограммы, но, предполагая, что вы согласны с предпосылкой, макрос, называемый crReturn, по крайней мере, выглядит так, как будто он возвращается.

Ответ 6

Я бы рассмотрел пример злоупотребления макросами по двум причинам: (1) Имя макроса не дает четкого представления о том, что он делает, особенно в том, что он может вернуться; (2) Макрос не синтаксически эквивалентен оператору. Код, например

  if (someCondition)
    CHECK_FREAD(whatever);
  else
    do_something_else();

не удастся. Мои предпочтительные изменения:

#define LOG_AND_RET_ERROR(msg) do {LOG_ERROR(msg); return DCD_BADREAD;} while(0)
  if (x==-1) LOG_AND_RET_ERROR(msg);

Ответ 7

Ну, если вы определяете короткую функцию, вам нужно набрать больше символов на каждом вызывающем сайте. То есть.

CHECK_FREAD(X, msg)

против.

if (check_fread(X, msg)) return DCD_BADREAD;

Ответ 8

В пределах пространства, которое оно было реализовано, возможно, оно было прекрасным (хотя на С++ оно обычно неодобрительно).

Самая яркая вещь, которая меня волнует, - это то, что кто-то из новоиспеченных разработчиков может это увидеть и подумать, чтобы сделать

if (CHECK_FREAD(...)) {
   /* do stuff here */
} else {
   /* error handle */
}

Что явно неправильно.

Кроме того, кажется, что msg не используется, если не потребляется/не ожидается в DCD_BADREAD, что ухудшает ситуацию...