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

/************************************************************************/
/*                                                                      */
/*                          MACRO CHECK_FREAD                           */
/*                                                                      */
/*  CHECK_FREAD is used to check the status of a file read.  It         */
/*   is passed the return code from read and a string to print out if   */
/*   an error is detected.  If an error is found, an error message is   */
/*   printed out and the program terminates.  This was made into a      */
/*   macro because it had to be done over and over and over . . .       */
/*                                                                      */
/************************************************************************/

#define CHECK_FREAD(X, msg)  if (X==-1) \
                 { \
                return(DCD_BADREAD); \
                 }

/*  Read in an 4                */
    ret_val = read(fd, &input_integer, sizeof(int32));

    CHECK_FREAD(ret_val, "reading an 4");
    CHECK_FEOF(ret_val, "reading an 4");

    if (input_integer != 4)
    {
        return(DCD_BADFORMAT);
    }

    /*  Read in the number of atoms         */
    ret_val = read(fd, &input_integer, sizeof(int32));
    *N = input_integer;

    CHECK_FREAD(ret_val, "reading number of atoms");
    CHECK_FEOF(ret_val, "reading number of atoms");

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

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

Ответ 2 · рейтинг 8 · Mark Ransom

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

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

Ответ 3 · рейтинг 5 · bta

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

Я бы сказал, что в целом вам следует избегать использования 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 · рейтинг 2 · Jens Gustedt

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

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

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

Ответ 5 · рейтинг 2 · Steve Jessop

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

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. Конечно, assert делает что-либо, только если NDEBUG не установлен, поэтому он не годится для обработки ошибок, если вы планируете отдельные сборки отладки и выпуска.

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

if (retval == -1) return DCD_BADREAD;

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

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

Макросы, которые выполняют поток управления, могут быть неприемлемыми, но я думаю, только в тех случаях, когда они выглядят как поток управления и точно задокументированы. На ум приходят макросы для совместной подпрограммы Саймона Тэтхема - некоторым людям в первую очередь не нравятся совместные подпрограммы, но, если вы принимаете эту предпосылку, макрос с именем crReturn по крайней мере выглядит так, как будто он вернется. .

Ответ 6 · рейтинг 1 · supercat

Я бы рассмотрел пример злоупотребления макросом по двум причинам: (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 · maxschlepzig

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

CHECK_FREAD(X, msg)

vs.

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

Ответ 8 · ezpz

В ограниченном пространстве это было реализовано, возможно, все было хорошо (хотя в C ++ это обычно не одобряется).

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

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

Что явно неверно.

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