Is a RETURN_ON_NULLPTR() macro bad code? (cpp:S960)

Clean Code Discussions
1

Hi,

we have some macros in our code like this:

#define RETURN_ON_NULLPTR(pointer) \
   do                              \
   {                               \
      if (nullptr == pointer)      \
      {                            \
         return ERR_INVALID_ARG;   \
      }                            \
   } while (false)

This (of course) triggers the issue: Function-like macros should not be used (cpp:S960) (or equivalent C-issue).

I know of know other way then to NOSONAR this. This macro cannot be turned into a static function, cause the return won`t return the caller. This is wanted here, hence the macro name.

  • Why are macros with return not an exception from this rule?
  • Or are “return-on-macros” generally also bad practice? Why?
4

Hello @KUGA2,

First, I’d like to share that the rule cpp:S960 is not absolute. Even if function-like macros are usually something to avoid, in some cases, they can increase the readability of your code. In that case, marking them as Won’t fix (or Accept with latest versions of our products) is perfectly appropriate.

Let’s discuss if there are better alternatives in this case because I think that beyond this rule, the main question is one of error management.

The most significant element I see in this macro is ERR_INVALID_ARG. The name refers to something that sounds like a programming error more than a normal condition, and I’m reluctant to clutter the main execution path with the handling of programming errors. If possible, I would prefer another mechanism, with the added advantage that the function return is now free to return meaningful values, instead of error codes.

Would it make sense for instance to abort if your precondition is violated (and let a higher-level system perform some recovery)? In that case, you can replace the macro with a simple function.

If you are in C++ and they are not disabled, maybe raising an exception could also be an option (many people are not fond of exceptions for programming errors, but they may provide a good level of error recovery).

There are of course variants still based on return code, with things like std::expected (C++23, but you can write your own). Those variants cannot be packaged in a function.

Now, another question is: Is the macro really worth it? The code it replaces is very simple, so why not simply write explicitly?

if (nullptr == pointer) { return ERR_INVALID_ARG; }

This comes with the added benefit of being able to test other conditions (if (value == 0) for instance) or to return other, more accurate, error codes.

What do you think?

6

I do not remember exactly but years ago we decided that the “wont fix” feature does not work for us. We use NOSONAR instead. With all its drawbacks, e.g. hiding other issues.
“wont fix” did not work, the server always forgot the setting because we were on a different branch or something like this. We are running SQ code built for multiple arch/OS, and therefore use a LOT of branches. Not to mention the 10+ release branches that we have to manage. And also we use branches for PR-Analysis, because SQ cannot have multiple PR analysis (per arch/OS).
(There is this feature Multiple code variants analysis on SonarQube for C, C++ and Objective-C but its incomplete (no multiple hosts) and it is not LTS yet, so admins wont update…)

Obviously this code was just a example. There exist a lot of different macros. They are also used on size missmatches, or permission problems. With different error codes. There are more complicated checks and maybe even multiple return values in one macro. Of course, again, i could do:

if(const auto e = evaluate(sth); if OK != e) return e;

and make evaluate a static function but this is a lot more code duplication. This is actually what we do to have SQ analyze more code but again, duplicating this is bad practice isnt it? Therefore we created the macros.

abort or exceptions are not feasible, because we are in realtime code.

We do not use C++23 but I do not see how this would omit duplicate code. Can you provide an example?

7

It is true that with your unconventional use of branches, issue tracking is probably all messed up, and NOSONAR might be the only working option. I was going to suggest multiple code variants, but you are already well aware of it. For the multiple hosts issue, an option could be do perform cross-compile from a single host. I don’t know how feasible it could be for you. And then convincing your admins that the latest version is better than the LTS.

Maybe instead of many macros, just one plus a series of functions that all return the same codes could be enough

#define PRE(cond) if(const auto e = cond; OK != e) return e;
PRE(evaluate(sth))

The expected feature of C++23 will not solve the macro issue. It may solve another one though. With the pattern of using the return channel of the function for error codes, this channel is no longer available to return actual values. std::expected allows you to use this return channel for both.

For instance, if you currently want a function that returns a double, since the return value is used for error code, you’ll write it that way:

ErrorCode f(int input, double &output);
double d;
auto err = f(42, d);
if (err != OK)...

With expected, you could write it that way:

expected<double> f(int input);
auto d = f(42);
if (!d) ...

And it also comes with functions like value_or, and_then, and or_else that can make the code to handle the error condition less noisy.

8

I actually did a talk that does a deep dive into this (two talks in fact). They were a few years ago now - before std::expected was standardized, so the names changed a bit - but this bit is close to what we’ve been talking about here and might be useful:

1 Like
9

Ok, thank you. I think we can reduce most macros to this. Or at least refrain from creating new ones.

And now I see what you ment with std::expected. That indeed is easier to read and less duplication. Sadly we cannot use it broadly cause we are limited to C-apis (and C++17).

10

The C-APIs would be an issue, but elsewhere you could use GitHub - TartanLlama/expected: C++11/14/17 std::expected with functional-style extensions which works with C++17 (even back to C++11).

12

I have tried std//tl::expected now.
I have also worked with monadic methods, which does eliminates the if(error) return; duplication but brings other duplications (and_then(), lambda overhead: []{return}) which is subjectively worse.

I must say, that I prefer the “PRE” solution from above. This is less code and imho easier to comprehend. Especially for “traditional developers” that need to review or maintain the code.
Of course you need the correct structure for this to work (RAII cleanup of unneeded objects), but then using the PRE macro is the best compromise.

So I come to the conclusion, that minimal macros when deduplication error handling are not bad code and can be rightfully Accepted (or Won’t Fix or NOSONAR).

13

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.