这是宏滥用吗？

Question

我正在对一些代码进行逆向工程并遇到了这个......

/************************************************************************/
/*                                                                      */
/*                          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++ 编程领域，而且我一开始就很少使用宏，但是这种宏是滥用吗？

我读到了... When are C++ macros beneficial?

...听起来不像任何犹太洁食的例子，所以我的猜测是“是”。但我想获得更明智的意见，而不仅仅是做出有根据的猜测...... :)

...errr，使用某种包装不是更好吗？

Answer 1

因为不一样。如果你把同样的东西放到一个函数中，你会得到一个短函数，如果X 等于-1，则返回DCD_BADWRITE。然而，有问题的宏从 包含 函数返回。示例：

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

将扩展为：

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

但是，如果它是一个函数，那么外部函数 (foobar) 会很高兴地忽略结果并返回 0。

一个看起来很像函数，但在关键方面表现不同的宏是一个主要的禁忌 IMO。我会说是的，它是宏滥用。

Answer 2

这不能作为函数完成，因为return 是从调用函数返回的，而不是宏中的块。

这种东西给宏起了个坏名声，因为它隐藏了一个重要的控制逻辑。

Answer 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，但是重用常见的错误处理代码而不是一系列嵌套块和条件变量可能是一个合适的用例。只需保持简洁易读（每个函数一个标签，并保持错误处理代码简单）。

Answer 4

这是否是滥用是一个品味问题。但我发现它存在一些主要问题

1. 文档是完整的 错了
2. 实施非常危险 因为if 和一个可能的 悬空else问题
3. 命名并不意味着 是一个初步的回报 周边功能

所以这绝对是非常糟糕的风格。

Answer 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。他们应该做的是完全删除宏并更改所有调用站点 - 如果您更改函数的控制逻辑并更改它返回的内容，最好明显更改该函数的源。相反，他们所做的是做出尽可能少的改变，从而达到他们想要的结果。这在当时似乎是个好主意（遗憾的是，忘记更新 cmets 是一个常见错误）。但正如每个人似乎都同意的那样，它导致了非常狡猾的代码。

控制流的宏可以是非滥用的，但我认为只有在它们看起来像控制流并且被准确记录的情况下。 Simon Tatham 的协程宏浮现在脑海中 - 有些人一开始就不喜欢协程，但假设你接受这个前提，一个名为 crReturn 的宏至少 看起来返回。

Answer 6

我会考虑示例宏滥用有两个原因： (1) 宏的名称并不清楚它的作用，最明显的是它可能会返回； (2) 宏在语法上不等同于语句。类似代码

如果（某些条件） CHECK_FREAD（随便）； 别的 do_something_else();

会失败。我的首选更改：

#define LOG_AND_RET_ERROR(msg) 做 {LOG_ERROR(msg);返回 DCD_BADREAD;} 而（0） if (x==-1) LOG_AND_RET_ERROR(msg);

Answer 7

好吧，如果您定义一个短函数，那么您必须在每个调用站点输入更多字符。即

CHECK_FREAD(X, msg)

对比

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

Answer 8

在实现它的空间范围内，它可能还不错（尽管在 C++ 中它通常不被接受）。

让我担心的最明显的事情是一些新引入的开发人员可能会看到并考虑这样做

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

这显然是错误的。

此外，除非在 DCD_BADREAD 中消耗/预期，否则似乎不会使用 msg，这会使情况恶化......