Это злоупотребление макросом? - PullRequest
12 голосов
/ 02 сентября 2010

Я занимался реверс-инжинирингом некоторого кода и наткнулся на это ...

/************************************************************************/
/*                                                                      */
/*                          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); \
                 }

По сути, определенный набор функций в этом файле вызывает его всякий раз, когда они выполняют (c-read) проверку результатаза ошибку.У них также есть аналогичная проверка для EOF ... По сути, насколько я могу судить, они делают это таким образом, чтобы вставлять возвраты ошибок в середине их функции в кучу мест.

Например

/*  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");

Теперь я только возвращаюсь к программированию на c / c ++, и я никогда прежде не использовал много макросов, но разве это злоупотребление макросами?

Я читал это ... Когда полезны макросы C ++?

... и это не похоже ни на один из кошерных примеров, поэтому я думаю, что "ДА".Но я хотел получить более информированное мнение, а не просто делать обоснованные предположения ...:)

... ошибаться, не лучше ли использовать какую-то упаковку?

Ответы [ 8 ]

22 голосов
/ 02 сентября 2010

Потому что это не то же самое. Если вы поместите то же самое в функцию, у вас будет короткая функция, которая возвращает 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.

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

8 голосов
/ 02 сентября 2010

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

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

5 голосов
/ 03 сентября 2010

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

Я бы сказал, что в целом вам следует избегать использования 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, но повторное использование общего кода обработки ошибок вместо ряда вложенных блоков и условных переменных может быть подходящим вариантом использования для этого.Просто держите его в чистоте и удобочитаемом виде (по одной метке на функцию, а код обработки ошибок - простым).

2 голосов
/ 02 сентября 2010

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

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 указывает на ошибки, что широко понимают программисты на Си. Если ничего другого, эта проверка полностью игнорирует тот факт, что read может выдать меньше данных, чем вы просили, без видимой причины. Это не ошибка макроса напрямую, но вся идея макроса, похоже, исходит из плохой проверки ошибок.

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

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

2 голосов
/ 02 сентября 2010

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

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

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

1 голос
/ 03 сентября 2010

Я бы рассмотрел пример злоупотребления макросами по двум причинам: (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);
0 голосов
/ 02 сентября 2010

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

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

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

Что явно не так.

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

0 голосов
/ 02 сентября 2010

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

CHECK_FREAD(X, msg)

против

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