views:

457

answers:

9

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(int32));
    *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... http://stackoverflow.com/questions/96196/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?

+18  A: 

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.

tdammers
What's the alternative if they're reading a ton of values and want to eliminate the repetitive code? Is there a good way of doing what they're doing (see my edited post for an example case)? About the only thing I can think of offhand, is they should have split the reading process up into different parts as its very long as the file has different sections...
Jason R. Mick
In C++, exceptions. Put the code into a small function which, instead of abusing return values to report failure, throws on error. Then in the using code, put the entire thing in a `try` / `catch` and return DCD_BADWRITE from the `catch`.If it's plain C, then `setjmp` - style error handling is probably a better solution.And then, maybe the hypothetical program would be better off using a loop in the first place.
tdammers
@Jason: I just don't think it's legitimate to want to eliminate the repetitive code here. Especially once you account for the fact that all the messages are completely redundant, their "post-elimination" code isn't even notably shorter than `if (retval != -1) return DCD_BADREAD`. It certainly isn't any easier to write or understand.
Steve Jessop
Saying not use a macro in the _only_ case that its worth using (where you need textual substitution, not a function) is a little ridiculous.
mathepic
+8  A: 

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.

Mark Ransom
@Mark Ransom: In C++, one should use "throw" for this type of thing. In C, a sensibly-named macro is often the best alternative. See my full answer for more info.
supercat
@supercat, I agree good naming would help.
Mark Ransom
A: 

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;
maxschlepzig
A: 

They're using a MACRO to develop a custom control structure for which there is no applicable alternative within the confines of the C++ language. The return statement is what matters most here as it short-circuits the logic of whatever method it resides within.

wheaties
There is a superior alternative in C++ (i.e. throw) but it's unavailable in C.
supercat
No, throw does not accomplish the same thing. It does leave the method scope but it does not do so cleanly. You still have to catch it and that requires extra clean up which this macro is attempting to simplify.
wheaties
@wheaties: True, this style of return is not identical to throw, but enclosing within a "catch" block the function which uses the macro would yield essentially the same result as the macro. Unless the quirky returns happen very frequently, throw/catch is probably superior. Incidentally, a "goto" might be better than a quirky return, since the target label would serve as a visible indication that something's up, and would ensure the macro could only be used in functions where it was "expected".
supercat
This is, perhaps, one of the few times I might agree with the use of a goto. However, I have a fundamental disagreement with you on the other portion. We'll just have to agree to disagree here.
wheaties
A: 

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...

ezpz
Newly introduced developers will make silly mistakes, like using the return value of a function (or result of an expression) without bothering to look up what it actually is. Not much point trying to account for that.
Steve Jessop
@Steve: On the other hand, presenting them with particularly confusing macros seems unkind and counterproductive. Particularly when the macro is badly formed and the documentation misleading.
David Thornley
True, but whether it's a good macro or a bad one (and I agree this is a bad one), the same could be said of *any* macro that doesn't expand to give an expression: "if somebody writes `if (MACRO(...)) { ... } else { ... }`, then it will go wrong".
Steve Jessop
+1  A: 

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.

Jens Gustedt
+2  A: 

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.

Steve Jessop
@Steve Jessop: Most likely, during debugging it's useful to enable code which logs the value in msg and returns. During production, the logging code is unnecessary. Leaving the macro in place allows the logging to be re-enabled should it be necessary.
supercat
@supercat: So the documentation describes debugging behaviour for which no code exists in the file, but that the debugging programmer should implement and insert when required? Instead of the production behaviour for which code exists? Then yes, that is undoubtedly colossal abuse ;-)
Steve Jessop
@Steve Jessop: Most likely the logging code did exist at some point in the past, but was removed. My preference would have been to use a nested macro for the logging part, and use a LOGGING_ENABLE flag to either define that nested macro to log something or do nothing.
supercat
+2  A: 

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

bta
A: 

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