81

Sometimes a local variable is used for the sole purpose of checking it in an assert(), like so -

int Result = Func();
assert( Result == 1 );

When compiling code in a Release build, assert()s are usually disabled, so this code may produce a warning about Result being set but never read.

A possible workaround is -

int Result = Func();
if ( Result == 1 )
{
    assert( 0 );
}

But it requires too much typing, isn't easy on the eyes and causes the condition to be always checked (yes, the compiler may optimize the check away, but still).

I'm looking for an alternative way to express this assert() in a way that wouldn't cause the warning, but still be simple to use and avoid changing the semantics of assert().

(disabling the warning using a #pragma in this region of code isn't an option, and lowering warning levels to make it go away isn't an option either...).

1

17 Answers 17

64

We use a macro to specifically indicate when something is unused:

#define _unused(x) ((void)(x))

Then in your example, you'd have:

int Result = Func();
assert( Result == 1 );
_unused( Result ); // make production build happy

That way (a) the production build succeeds, and (b) it is obvious in the code that the variable is unused by design, not that it's just been forgotten about. This is especially helpful when parameters to a function are not used.

Sign up to request clarification or add additional context in comments.

8 Comments

Until Result is later used, and then it's just confusing :) But nice suggestion +1.
also, Func is still called (unless the optimizer knows it has no side-effects)
This is a good idea. I am using it for parameters to functions that are not used, but using it for assert()s is certainly a nice option as well. Maybe a macro called, say, FOR_ASSERT() can be used instead to make the reasoning clear. Thanks!
@xtofl: That's a point, but almost certainly you want Func() to be called -- otherwise you could have just written "assert(Func() == 1);" in the first place.
You can also make your assert a macro like: #define assert(X,Y) __assert(X,Y); _unused(X) where X is your variable and Y the condition. Usage in your case: assert(Result, 1);
|
41

As of C++17, the variable can be decorated with an attribute.

[[maybe_unused]] int Result = Func();
assert( Result == 1 );

See https://en.cppreference.com/w/cpp/language/attributes/maybe_unused for details.

This is better than the (void)Result trick because you directly decorate the variable declaration, rather than add something as an afterthought.

1 Comment

It is worth mentioning that "recent C++" == since C++17
37

I wouldn't be able to give a better answer than this, that addresses that problem, and many more:

Stupid C++ Tricks: Adventures in assert

#ifdef NDEBUG
#define ASSERT(x) do { (void)sizeof(x);} while (0)
#else
#include <assert.h>
#define ASSERT(x) assert(x)
#endif

2 Comments

For the lazy, in release builds: #define ASSERT(X) do { (void)sizeof(x); } while(0)
I like this better because it works even in the case where x is not a variable. So you don't have to use a different macro depending on what you assert... Also the link is definitely wort the read.
10

You could create another macro that allows you to avoid using a temporary variable:

#ifndef NDEBUG
#define Verify(x) assert(x)
#else
#define Verify(x) ((void)(x))
#endif

// asserts that Func()==1 in debug mode, or calls Func() and ignores return
// value in release mode (any braindead compiler can optimize away the comparison
// whose result isn't used, and the cast to void suppresses the warning)
Verify(Func() == 1);

3 Comments

This solves the original problem, is very neat and easy to understand, and also avoids a whole class of bugs where debug builds execute different code to release builds. Is there a reason it doesn't have more up votes? Are there gotchas anyone is aware of?
This doesn't work if you do int s = Fun(); Verify(s == 1) because many compilers will complain that s is set but unused. Even the (void)(x) trick doesn't work (Intel compiler, 2017 version).
@MarkLakata: That seems like a defect in the Intel compiler. I haven't seen that type of problem with any compilers I've ever worked with, though admittedly I've never used the Intel compiler. I'd wager that you could work around it by explicitly adding a useless (void)s afterwards, but that's clearly suboptimal.
10
int Result = Func();
assert( Result == 1 );

This situation means that in release mode, you really want:

Func();

But Func is non-void, i.e. it returns a result, i.e. it is a query.

Presumably, besides returning a result, Func modifies something (otherwise, why bother calling it and not using its result?), i.e. it is a command.

By the command-query separation principle, Func shouldn't be a command and a query at the same time. In other words, queries shouldn't have side effects, and the "result" of commands should be represented by the available queries on the object's state.

Cloth c;
c.Wash(); // Wash is void
assert(c.IsClean());

Is better than

Cloth c;
bool is_clean = c.Wash(); // Wash returns a bool
assert(is_clean);

The former doesn't give you any warning of your kind, the latter does.

So, in short, my answer is: don't write code like this :)

Update (1): You asked for references about the Command-Query Separation Principle. Wikipedia is rather informative. I read about this design technique in Object Oriented Software Construction, 2nd Editon by Bertrand Meyer.

Update (2): j_random_hacker comments "OTOH, every "command" function f() that previously returned a value must now set some variable last_call_to_f_succeeded or similar". This is only true for functions that don't promise anything in their contract, i.e. functions that might "succeed" or not, or a similar concept. With Design by Contract, a relevant number of functions will have postconditions, so after "Empty()" the object will be "IsEmpty()", and after "Encode()" the message string will be "IsEncoded()", with no need to check. In the same way, and somewhat symetrically, you don't call a special function "IsXFeasible()" before each and every call to a procedure "X()"; because you usually know by design that you're fulfilling X's preconditions at the point of your call.

13 Comments

This advice is too general. What about list.Remove? Here you may need to have the return value whether an element was removed. list.WasSuccessfullyRemoved() would be bogus.
Returning an error code from a function is perfectly fine, but you seem to damn the practice.
@Daniel: Could you explain the rationale for the "command-query separation principle"? A link would be fine. Thanks!
@Daniel: Thanks for the update. I can see the advantages in this. OTOH, every "command" function f() that previously returned a value must now set some variable last_call_to_f_succeeded or similar, possibly causing problems for re-entrant/multithreaded code (as noted in the article). But yes, CQS definitely seems like a valid, logic-simplifying approach worth applying where possible.
@j_random_hacker: As with all techniques, it is always subject to evaluation. In many cases, I design like that, but I admit that sometimes I use the other strategy, especially in cases where nothing can be asserted about the result, e.g. "FindFile()" or "UpdateConfigurationFile()". It's just another technique in your toolset, to be used judiciously. However, stay tuned for update (2) :)
|
6

With C++17 we can do:

[[maybe_unused]] int Result = Func();

though it involves a bit of extra typing compared to a assert substitution. See this answer.

Note: Added this because is the first google hit for "c++ assert unused variable".

Comments

4

You could use:

Check( Func() == 1 );

And implement your Check( bool ) function as you want. It may either use assert, or throw a particular exception, write in a log file or to the console, have different implementations in debug and release, or a combination of all.

1 Comment

I like this solution. However, for simplicity and concise, I would suggest assert(Func() == 1); The extra Check layer should be up to the programmer.
3

The simplest thing is to only declare/assign those variables if the asserts will exist. The NDEBUG macro is specifically defined if asserts won't be effected (done that way round just because -DNDEBUG is a convenient way to disable debugging, I think), so this tweaked copy of @Jardel's answer should work (cf. comment by @AdamPeterson on that answer):

#ifndef NDEBUG
int Result =
#endif
Func();
assert(Result == 1);

or, if that doesn't suit your tastes, all sorts of variants are possible, e.g. this:

#ifndef NDEBUG
int Result = Func();
assert(Result == 1);
#else
Func();
#endif

In general with this stuff, be careful that there's never a possibility for different translation units to be build with different NDEBUG macro states -- especially re. asserts or other conditional content in public header files. The danger is that you, or users of your library might accidentally instantiate a different definition of an inline function from the one used inside the compiled part of the library, quietly violating the one definition rule and making the runtime behaviour undefined.

Comments

2

You should move the assert inside the function before the return value(s). You know that the return value is not an unreferenced local variable.

Plus it makes more sense to be inside the function anyway, because it creates a self contained unit that has its OWN pre- and post-conditions.

Chances are that if the function is returning a value, you should be doing some kind of error checking in release mode on this return value anyway. So it shouldn't be an unreferenced variable to begin with.

Edit, But in this case the post condition should be X (see comments):

I strongly disagree with this point, one should be able to determine the post condition from the input parameters and if it's a member function, any object state. If a global variable modifies the output of the function, then the function should be restructured.

9 Comments

It could be that Func() is generic enough to be used in multiple places, but in this case the return value must be 1. In other cases, other return values may be allowed, so you need to have the assertion here. Unless you want to start passing parameters into Func() to indicate why you're calling it and then have it decide whether to assert, but that seems overly complicated.
Yes, I agree with Graeme Perrow's comment. This is the situation here.
I strongly disagree with this point, one should be able to determine the post condition from the input parameters and if it's a member function, any object state. If a global variable modifies the output of the function, then the function should be restructured.
Re: "Unless you want to start passing parameters into Func() to indicate why you're calling it" - Any function's input and current object state should determine its output. If there is any question of needing to pass why you're calling it to determine the return value, there are major design problems in play.
@Michael Burr: Yup I agree, it is not applicable if you don't have control of the func. But when you do it should be in there.
|
2

Most answers suggest using static_cast<void>(expression) trick in Release builds to suppress the warning, but this is actually suboptimal if your intention is to make checks truly Debug-only. The goals of an assertion macro in question are:

  1. Perform checks in Debug mode
  2. Do nothing in Release mode
  3. Emit no warnings in all cases

The problem is that void-cast approach fails to reach the second goal. While there is no warning, the expression that you've passed to your assertion macro will still be evaluated. If you, for example, just do a variable check, that is probably not a big deal. But what if you call some function in your assertion check like ASSERT(fetchSomeData() == data); (which is very common in my experience)? The fetchSomeData() function will still be called. It may be fast and simple or it may be not.

What you really need is not only warning suppression but perhaps more importantly - non-evaluation of the debug-only check expression. This can be achieved with a simple trick that I took from a specialized Assert library:

void myAssertion(bool checkSuccessful)
{
   if (!checkSuccessful)
    {
      // debug break, log or what not
    }
}

#define DONT_EVALUATE(expression)                                    \
   {                                                                 \
      true ? static_cast<void>(0) : static_cast<void>((expression)); \
   }

#ifdef DEBUG
#  define ASSERT(expression) myAssertion((expression))
#else
#  define ASSERT(expression) DONT_EVALUATE((expression))
#endif // DEBUG

int main()
{
  int a = 0;
  ASSERT(a == 1);
  ASSERT(performAHeavyVerification());

  return 0;
}

All the magic is in the DONT_EVALUATE macro. It is obvious that at least logically the evaluation of your expression is never needed inside of it. To strengthen that, the C++ standard guarantees that only one of the branches of conditional operator will be evaluated. Here is the quote:

5.16 Conditional operator [expr.cond]

logical-or-expression ? expression : assignment-expression

Conditional expressions group right-to-left. The first expression is contextually converted to bool. It is evaluated and if it is true, the result of the conditional expression is the value of the second expression, otherwise that of the third expression. Only one of these expressions is evaluated.

I have tested this approach in GCC 4.9.0, clang 3.8.0, VS2013 Update 4, VS2015 Update 4 with the most harsh warning levels. In all cases there are no warnings and the checking expression is never evaluated in Release build (in fact the whole thing is completely optimized away). Bare in mind though that with this approach you will get in trouble really fast if you put expressions that have side effects inside the assertion macro, though this is a very bad practice in the first place.

Also, I would expect that static analyzers may warn about "result of an expression is always constant" (or something like that) with this approach. I've tested for this with clang, VS2013, VS2015 static analysis tools and got no warnings of that kind.

1 Comment

It does emit a warning for Intel 2017.4 icpc, if you use ASSERT( a == 1) (error #174: expression has no effect)
1

This is a bad use of assert, IMHO. Assert is not meant as an error reporting tool, it's meant to assert preconditions. If Result is not used elsewhere, it's not a precondition.

4 Comments

In the current place in the code where Func() is called, it is a "can't happen" incident that this function return anything which isn't 1. So an assert() is appropriate here. This isn't error reporting / error checking - it is sanity checking.
So presumably your application is interested in the side-effects, rather than the return code (as the release code will ignore it)? In that case, I would place an assertion on the side-effects.
assert can be used for checking preconditions, postconditions, class invariants or intermediate results needed in the implementation.
@Neil: Suppose Func() is actually printf(), and the return value is the number of characters output -- how could you test the side effects other than by checking the return value?
1

Certainly you use a macro to control your assert definition, such as "_ASSERT". So, you can do this:

#ifdef _ASSERT 
int Result =
#endif /*_ASSERT */
Func();
assert(Result == 1);

1 Comment

You should just use the NDEBUG macro for this. Its standard-specified behavior is to be set exactly when assert() is disabled.
0
int Result = Func();
assert( Result == 1 );
Result;

This will make the compiler stop complaining about Result not being used.

But you should think about using a version of assert that does something useful at run-time, like log descriptive errors to a file that can be retrieved from the production environment.

2 Comments

Result; is another warning - which is unused value or has no effect.
@Mykola Golubyev: Not on VC9 @ Warning Level 4. Which compiler are you using?
0

I'd use the following:

#ifdef _DEBUG
#define ASSERT(FUNC, CHECK) assert(FUNC == CHECK)
#else
#define ASSERT(FUNC, CHECK)
#endif

...

ASSERT(Func(), 1);

This way, for release build, the compiler don't even need to produce any code for assert.

5 Comments

In release Func() is not called
For the else case you need "#define ASSERT(FUNC, CHECK) FUNC
If Func() is required only to return a value for asserting, then in release, the call is not necessary. But yes, otherwise it is necessary and I missed that.
How does this apply to assertions that are not equality, i.e. assert( foo.length() < N) ?
@MarkLakata: Good question. My solution doesn't cover that.
0

If this code is inside a function, then act on and return the result:

bool bigPicture() {

   //Check the results
   bool success = 1 != Func();
   assert(success == NO, "Bad times");

   //Success is given, so...
   actOnIt();

   //and
   return success;
}

Comments

0
// Value is always computed.  We also call assert(value) if assertions are
// enabled.  Value is discarded either way.  You do not get a warning either
// way.  This is useful when (a) a function has a side effect (b) the function
// returns true on success, and (c) failure seems unlikely, but we still want
// to check sometimes.
template < class T >
void assertTrue(T const &value)
{
  assert(value);
}

template < class T >
void assertFalse(T const &value)
{ 
  assert(!value);
}

Comments

0

I haven't succeeded in using [[maybe_unused]] but you can use the unused attribute

int Result __attribute__((__unused__)) = Func();

gcc Variable-Attributes

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.