1
votes

I'm trying to clean up some legacy code that contains hundreds of functions with bodies that look like this:

void functionWithSideEffect(Foo* foo)
{
    if (foo)
    {
        // Use `foo' to warm the planet
        ...
    }
}

Obviously, silently failing if a precondition check fails isn't the best idea, so I'd like to refactor this to:

int functionWithSideEffect(Foo* foo)
{
    RETURN_IF_FALSE(foo != NULL, "foo is NULL in functionWithSideEffect!");

    // Use `foo' to warm the planet
    ...
}

The following macro seems to work fine for functions that don't return a value:

#define RETURN_IF_FALSE(cond, msg) \
  do { \
      if (!(cond)) { \
          LOG("%s\n", (msg)); \
          assert((cond)); \
      } \

      return; \
  } while(0)

And it has the following desirable properties:

  • It is clear and succinct in usage
  • It does not silently fail
  • It will crash in debug builds, but attempt to soldier on in release builds

(Admittedly, for functions returning void, soldiering on in release builds may not always be 'desirable'.)

For functions that do return a value, this macro does the trick:

#define RETURN_VALUE_IF_FALSE(cond, msg, retval ) \
  do { \
      if (!(cond)) { \
          LOG("%s\n", (msg)); \
          assert((cond)); \
      } \
      return(retval); \
  } while(0)

My question is: is it possible to write a single RETURN_IF_FALSE macro to handle both void functions and functions returning a value? I sat down to attempt something using a varargs macro and quickly discovered I'm not very good at writing complex macros. I started out with this test program:

#include <stdio.h>
#include <assert.h>

#define RETURN_IF_FALSE(cond, msg, ... ) \
  do { \
      if (!(cond)) { \
          fprintf(stderr, "%s\n", (msg)); \
          assert((cond)); \
      } \
      return (##__VA_ARGS__); \
  } while(0)


int main()
{
    RETURN_IF_FALSE(1 < 0, "1 is not less than 0!", -1);
    return 0;
}

Perhaps not surprisingly, it generated the following compile error:

g++     macro_test.cpp   -o macro_test
macro_test.cpp:10:14: error: pasting "(" and "-" does not give a valid preprocessing token
       return (##__VA_ARGS__); \
              ^
macro_test.cpp:16:5: note: in expansion of macro ‘RETURN_IF_FALSE’
     RETURN_IF_FALSE(1 < 0, "1 is not less than 0!", -1);
     ^

Is it even possible to cover both cases with a single macro? I'm using gcc 4.8.1 on Linux. (I can compile with -std=c++11, if it helps...)

UPDATE: To bring this full-circle, here's the implementation I ultimately wound up with based on @Turix's answer and a suggestion from @Deduplicator to move the assert() call up to avoid a double evaluation of the conditional in the 'sunny day' case:

#define RETURN_IF_FALSE(cond, ... ) \
  do { \
      if (!(cond)) { \
          const char* msg = \
              "Pre-condition '" #cond "' not met, returning " #__VA_ARGS__ "..."; \
          LOG("%s\n", msg); \
          assert((cond)); \
          return __VA_ARGS__; \
      } \
  } while(0)

(I decided it wasn't really all that necessary/useful to allow setting of a 'free form' message string, so I just generated a canned one from the condition...)

2
Why would you want to do this? If the pointer is null, you will get undefined behavior. If you want it to try and soldier on, throw an exception and something can catch it and try the next thing. - Neil Kirk
Why do you use the concatenation operator ##? Leave it out, and it will compile. - dyp
If it's a precondition error, just abort/terminate. Don't return to the caller. The calling code is broken. - James McNellis
Just a tip: Only evaluate cond once. - Deduplicator
Why don't you just add an else clause to the if(foo) statement and make noise about the failure there? - celtschk

2 Answers

2
votes

Just replace this part of your macro return (##__VA_ARGS__); with return __VA_ARGS__ ; and I think it should do what you want (assuming that what you would pass for the return value isn't a complex expression -- if it is, you would need to pre-wrap the parameter with parentheses).

1
votes

I got this to work.

#include <stdio.h>
#define RET_IF_FALSE(x, y, z) if (!x) { printf(y); return z; }


int a(int *p)
{
    RET_IF_FALSE(p, __FUNCTION__, 0);

    return *p;
}

void b(int *p)
{
    RET_IF_FALSE(p, __FUNCTION__, );
}


int main()
{
    int x;

    x = a(&x);
    b(&x);

    x = a(NULL);
    b(NULL);

    return 0;
}

It may not be the prettiest solution with a trailing comma, and it isn't compliant with standards according to gcc's -pedantic option.

Using:

#define RET_IF_FALSE(x, y, ...) if (!x) { printf(y); return __VA_ARGS__; }

with the rest of the code the same works for gcc with pedantic and -std=c99, and with -std=c++11 in clang++ and g++. Not sure what MS compilers do, as their support for standards is a little less stellar at times (and I haven't got a Windows setup to test on at present).