Is this Macro Abuse?

I was reverse engineering some code and came across this...

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

Basically, the particular set of functions in this file call this whenever they do a (c-read) to check the result for an error. They also have a similar check for EOF... Basically as far as I can tell, they're doing it this way to insert returns on error in the middle of their function in a bunch of places.

e.g.

/*  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(in开发者_开发知识库t32));
    *N = input_integer;

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

Now I'm just getting back into c/c++ programming, and I never used macro's much in the first place, but is this macro abuse?

I read this... When are C++ macros beneficial?

...and it doesn't sound like any of the kosher examples, so my guess would be "YES". But I wanted to get a more informed opinion rather than just making educated guesses... :)

...errr, wouldn't it be better to use some sort of wrapping?

Because it's not the same. If you put the same thing into a function, you'll have a short function that returns DCD_BADWRITE if X equals -1. The macro in question however returns from the containing function. Example:

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

would expand to:

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

If, however, it were a function, the outer function (foobar) would happily ignore the result and return 0.

A macro that looks so much like a function, but behaves differently in a crucial way, is a major no-no IMO. I'd say yes, it is macro abuse.

This can't be done as a function because the return is returning from the calling function, not the block within the macro.

This is the sort of thing that gives macros a bad name, because it's hiding a vital piece of control logic.

I'm on the fence about whether to call such a macro *abuse*, but I will say that it is not a good idea.

I would argue that in general, you should avoid using return in a macro. It's hard to create a macro that handles the return correctly without making the control flow of the function that calls it awkward or hard to understand. You definitely don't want to "accidentally" return from a function without realizing it. That one can be annoying to debug.

Plus, this macro can cause problems with code flow depending on how it is used. For example, the code

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

won't do what you would think it would (the else becomes associated with the if inside the macro). The macro needs to be enclosed in { } to make sure it doesn't alter any surrounding conditionals.

Also, the second argument is not used. The obvious problem here is the implementation not matching the documentation. The hidden problem is when the macro is invoked like this:

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

You expect the pointer to move to the next array element after the macro is called. However, the second parameter isn't used so the increment never happens. This is yet another reason why statements with side effects cause problems in macros.

Instead of using a macro to do all of this, why not write a function to encapsulate the read and the error checks? It can return zero on success or an error code. If the error-handling code is common between all of the read blocks, you can do something like this:

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;
}

As long as all of your calls to read_helper assign their return value to ret, you can re-use the same block of error handling code throughout the whole function. Your function print_error_message_for_code would simply take an error code as input and use a switch or an array to print out an error message that corresponds to it.

I admit that many people are afraid of goto, but re-using common error handling code instead of a series of nested blocks and condition variables can be an appropriate use case for it. Just keep it clean and readable (one label per function, and keep the error-handling code simple).

Whether or not this is abuse is a matter of taste. But I see some principal problems with it

1. the documentation is completely wrong
2. the implementation is very dangerous because of the if and a possible dangling else problem
3. the naming doesn't suggest that is is a preliminary return of the surrounding function

so it is definitively very bad style.

If the macro is in a source file and used only in that file, then I find it a bit less offensive than if it's off in some header somewhere. But I don't much like a macro that returns, (especially one that's documented to terminate the app, and actually returns), and still less one that conditionally returns, because it makes it pretty easy to create the following memory leak:

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

If I believe the documentation, I have no grounds to suspect that this code leaks memory whenever the read fails. Even if the documentation were correct, I do prefer control-flow in C to be glaringly obvious, which means not hidden by macros. I'd also prefer my code to look vaguely consistent between cases where I have resources to clean up, and cases where I don't, rather than using the macro for one but not the other. So the macro isn't to my taste even if it's not definitively abusive.

To do what the documentation says, I would prefer to write something like:

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

with check being a function. Or use an assert. Of course an assert only does anything if NDEBUG isn't set, so it's no good for error-handling if you're planning separate debug and release builds.

To do what the macro actually does, I would prefer to skip the macro entirely, and write:

if (retval == -1) return DCD_BADREAD;

That's hardly a long line of code, and there's no real benefit in commoning-up different instances of it. It's also probably not a good idea to think you will improve things by encapsulating/hiding the existing, well-documented means by which read indicates errors, that's widely understood by C programmers. If nothing else, this check completely ignores the fact that read can produce less data than you asked for, for no observable reason. That's not the macro's fault directly, but the whole idea behind the macro seems to come from bad error-checking.

Probably what's happened here is that they used to have a macro which terminated, then they decided they didn't want to abort the program immediately when failures occurred, instead they wanted to return this error code DCD_BADREAD. What they should have done was to remove the macro entirely and changed all the calling sites - if you change the control logic of a function, and change what it returns, it's best to visibly change the source of that function. What they did instead was make the minimum possible change that gave the result they wanted. It probably seemed like a good idea at the time (and forgetting to update the comments is sadly a common error). But as everyone seems to agree, it has resulted in very dodgy code.

Macros that do control flow can be non-abusive, but I think only in situations where they look like control flow, and are documented accurately. Simon Tatham's co-routine macros spring to mind - some people don't like co-routines in the first place, but assuming you accept the premise, a macro called crReturn at least looks like it's going to return.

I would consider the example macro abuse for two reasons: (1) The name of the macro does not make clear what it does, most notably that it might return; (2) The macro is not syntactically equivalent to a statement. Code like

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

will fail. My preferred change:

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

Well, if you define a short function then you have to type more characters at each calling site. I.e.

CHECK_FREAD(X, msg)

vs.

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

In the confines of the space it was implemented it may have been fine (although it is usually frowned upon in C++).

The most glaring thing about this that worries me is that some newly introduced developer might see that and think to do

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

Which is obviously wrong.

Also, it seems that msg is not used unless consumed/expected in DCD_BADREAD which worsens the situation...