这是宏滥用吗？

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?

...听起来不像任何一种正常的例子，所以我的猜测是“是”。但是我想得到一个更有见地的意见，而不仅仅是做出有根据的猜测... :)

...额，使用一些包装方法不会更好吗？

- Jason R. Mick

---

这个宏没有按照注释中的描述打印错误消息。这是完整的代码吗？ - Kirill V. Lyadvinsky

是的，这就是完整的代码。给ChrisF的话，可以看一下我添加的示例。 - Jason R. Mick

为什么没有使用'msg'参数？注释与实现不符，这意味着至少有一个存在错误，但很可能是两者都有问题。最好编写一个size_t checked_fread(void *buffer, size_t itemsize, size_t numitems, FILE *fp, const char *msg)函数并始终使用它。有趣的是推测为什么要检查-1；fread()在出错时返回0或短计数-而返回类型为size_t，几乎所有目的都不允许返回值为-1。 - Jonathan Leffler

但这是否被视为滥用了某些东西？这是我的问题。@Jonathan，我不知道为什么他们不使用msg...是健忘吗？懒惰？混乱？你来决定！但你绝对是对的，这很奇怪... - Jason R. Mick

他们在测试代码时可能使用了“msg”属性，并在发布之前删除了打印代码。起初我认为这是一种将注释插入到代码中的方法，但为什么不使用诚实的/* 注释 */呢？ - teukkam

滥用与误用？很难说...因为msg未被使用，所以有点无意义。显然，关于fread()的评论与read()不相关。由于我们看不到CHECK_FEOF()，我们无法知道它的作用。不过，在我看来，编写一个简单的函数来处理这两种情况会更好：它可以包装read()函数（系统调用）并将错误状态与其他内容分离出来：if ((retval = wrapped_read(fd, &input_integer, sizeof(int32), "reading a 4")) != DCD_NOERROR) return retval;。如果对4的测试普遍存在，我会进一步封装它。我认为这是宏滥用。 - Jonathan Leffler

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。

一个看起来非常像函数但关键行为方式不同的宏，是我认为不能这样做的。我会说是的，这是宏的滥用。

- tdammers

---

如果他们正在阅读大量值并想要消除重复代码，有什么替代方案？ 是否有一种好的方法来执行他们正在做的事情（请参见我的编辑过的帖子中的示例用例）？ 我能够立即想到的唯一一件事是，他们应该将阅读过程拆分为不同的部分，因为该文件非常长且具有不同的部分... - Jason R. Mick

在C++中，异常处理是一个好的选择。将代码放入一个小函数中，而不是滥用返回值来报告错误，当出现错误时抛出异常。然后在使用代码中，将整个内容放入try/catch块中，并从catch中返回DCD_BADWRITE。如果是纯C，则使用类似于setjmp的错误处理可能是更好的解决方案。或许，在第一次使用循环时，假设性程序会更好。 - tdammers

@Jason：我觉得在这里想要消除重复的代码并不合理。特别是考虑到所有的消息都是完全冗余的事实，他们的“消除后”代码甚至比if (retval != -1) return DCD_BADREAD并没有明显更短。它肯定不会更容易编写或理解。 - Steve Jessop

说在唯一值得使用宏的情况下不要使用它（需要文本替换而不是函数）有点荒谬。 - alternative

Answer 2

这无法作为函数来完成，因为return返回的是调用函数而不是宏块内部。

这种做法会让宏函数名声扫地，因为它掩盖了关键的控制逻辑。

- Mark Ransom

---

在C++中，应该使用“throw”来处理这种情况。在C语言中，一个名字合理的宏通常是最好的替代方案。请查看我的完整答案以获取更多信息。 - supercat

Answer 3

我对于这种宏是否算作“滥用”持保留意见，但我可以说这不是一个好主意。一般来说，我建议避免在宏中使用 return。很难创建一个正确处理 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，但是重复使用常见的错误处理代码而不是一系列嵌套的块和条件变量可能是其适当的使用情况。只需保持代码简洁易读（每个函数一个标签，并保持错误处理代码简单）。

- bta

Answer 4

这是否算是滥用的问题取决于个人看法。但我认为它存在一些主要问题：

1. 文档完全错误
2. 由于 if 和可能出现的悬挂 else 问题，实现非常危险
3. 命名并没有表明它是周围函数的初步返回值

因此，这绝对是非常糟糕的风格。

- Jens Gustedt

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被设置了，那么assert将不起作用，所以如果你计划单独进行调试和发布构建，则无法用它进行错误处理。

为了实现宏的实际功能，我更喜欢跳过宏，直接编写：

if (retval == -1) return DCD_BADREAD;

那只是一小段代码，没有将其通用化的必要。同时，通过封装/隐藏现有的、经过良好文档化的read错误表示方式来改进事物也不是一个好主意，这种方式已经被C程序员广泛理解。此外，这个检查完全忽略了read可能会因为无法观察到的原因产生比你所请求的数据更少的情况。这并不是宏的错，但整个宏的想法似乎来自于糟糕的错误检查。

可能发生的情况是他们曾经使用一个终止宏，然后他们决定当出现故障时不立即中止程序，而是返回错误代码DCD_BADREAD。他们应该完全删除宏并更改所有调用站点——如果您更改函数的控制逻辑并更改其返回值，最好明显地更改该函数的源代码。相反，他们所做的是尽可能少的更改以达到他们想要的结果。当时这可能看起来是个好主意（遗忘更新注释很常见）。但正如每个人都认为的那样，这导致了非常糟糕的代码。

控制流程的宏可以是非滥用的，但我认为只有在它们看起来像控制流程，并且有准确的文档记录的情况下才是如此。Simon Tatham的协程宏就是一个例子——有些人一开始就不喜欢协程，但是假设您接受了这个前提，一个名为crReturn的宏至少看起来像要返回。

- Steve Jessop

---

在调试期间，启用记录 msg 值并返回的代码非常有用。在生产环境中，记录代码是不必要的。保留宏可以重新启用日志记录，如果有必要的话。 - supercat

@supercat：所以文档描述了在文件中不存在的调试行为，但是调试程序员应该在需要时实现和插入？而不是存在代码的生产行为？那么是的，这无疑是巨大的滥用；-) - Steve Jessop

很可能记录代码在过去的某个时间点存在，但已被删除。我更喜欢使用嵌套宏来处理记录部分，并使用LOGGING_ENABLE标志来定义该嵌套宏是记录某些内容还是什么都不做。 - supercat

Answer 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);

- supercat

Answer 7

如果你定义了一个简短的函数，那么每次调用时你就需要输入更多的字符。例如：

CHECK_FREAD(X, msg)

对比。

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

- maxschlepzig

Answer 8

在实现的空间范围内，这可能是可以的（尽管在C++中通常不被赞同）。

最让我担心的是，一些新加入的开发人员可能会看到这个并想去做。

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

这显然是错误的。

而且，似乎 msg 只有在消耗/预期到 DCD_BADREAD 时才被使用，这更加恶化了情况...

- ezpz

---

新入门的开发人员会犯一些愚蠢的错误，比如使用函数的返回值（或表达式的结果），而不费心去查看它实际上是什么。没有必要为此进行解释。 - Steve Jessop

@Steve：另一方面，向他们展示特别混乱的宏似乎是不友善和适得其反的。尤其是当宏形式不良且文档具有误导性时。 - David Thornley

真的，但无论它是一个好的宏还是一个坏的宏（我同意这是一个坏的宏），对于任何不扩展以给出表达式的宏都可以说同样的话：“如果有人写if (MACRO(...)) { ... } else { ... }，那么它会出错”。 - Steve Jessop