Spot the WTF -- ASSERT() gone horribly wrong
-
spotted in a "teach yourself C++"-type book:
#ifndef DEBUG
#define ASSERT(x)
#else
#define ASSERT(x)\
if (!(x)) {\
cerr << "ASSERT failed at " << __FILE__ << " line " << __LINE__ << endl;\
abort();\
}
#end
-
For the C++ - challenged among us, the WTF would be....? (I mean, besides manually defining assert)
-
Yeah, sure, just include <assert.h> but it seems about right.
Normal assert:
Should always work unless you define NDEBUG
Should include the function where it happened.But overall, looks like it works pretty much the same as the default linux assert.
-
No, the worst assert WTF I worked with was...
#ifdef NDEBUG
void assert(bool foo){
if(!foo){
cout << "Assert failed";
abort();
}
}
#endif
For some reason -DNDEBUG compiler option didn't seem to have any effect....
/And the problems were being caused by pieces of code like this:
assert(p != NULL);
if(p == NULL){
//fail this transaction gracefully, rather than bringing down the whole system.
}
-
The only thing that I can see wrong is that the non-debug version will not cause side-effects, whereas the debug one will.
For example:
[code]FILE *fp;
ASSERT(fp = fopen("infile.dat","r"));
[/code]
Will open the file in debug mode, but not in release mode!
-
Not sure if this is what the OP was thinking of, but
if (sign == POSITIVE)
ASSERT(x > 0);
else
ASSERT(x < 0);(Contrived I know, but you get the idea.)
EDIT: and I wouldn't really call it a WTF, since it's fairly subtle. Although putting it in a tutorial is pretty naughty.
-
Of course the solution to mine would fix both these problems:
[code]#define ASSERT(x) x[/code]
...and don't worry, if there are no side effects the compiler will optimize it to nothing (providing you got all your const funcs right!)
-
How does your version prevent the problem with dangling 'else'?
And the assert implementation from <cassert> header in VS (I don't remember 2003 or 2005) does not execute any side effects when compiled in Release mode.
-
@GettinSadda said:
The only thing that I can see wrong is that the non-debug version will not cause side-effects, whereas the debug one will.
For example:
[code]FILE *fp;
ASSERT(fp = fopen("infile.dat","r"));
[/code]
Will open the file in debug mode, but not in release mode!Actually, that's not wrong, that's it behaving exactly the same as standard assert. See the C language spec, section 7.2:
If NDEBUG is defined as a macro name at the
point in the source file where <assert.h> is included, the assert macro is defined
simply as
#define assert(ignore) ((void)0)
The real WTF would be writing code with side-effects inside an assert. That's daft!
-
Aha, the dangling semicolon does cause problems.
In MingW this is defined as:
#define assert(e) ((e) ? (void)0 : _assert(#e, __FILE__, __LINE__))
By the way, side effects are not important. Anything that can be evaluated as bool (e.g x = 10) is OK for the compiler. You just never put stuff that has side-effects in assert, obviously.
-
Okay, I had two main points, really. The first, someone already caught; the else gets misassigned in
if (foo) ASSERT(x);
else ASSERT(y);The other is that defining ASSERT() away to nothing has the potential to break things in unexpected ways if you use an ASSERT() in a place where a value is compulsory. For instance:
for (i = 0; ASSERT(i ...), i < n; ++ i) ...
(and there is a good reason to stick statements in before the conditional part of a for(;;) – that's the only place you can put some code such that it gets executed before you enter the loop the first time, between every iteration, and after the last iteration.
Generally, these'd be possibly acceptable, but definitely not in a tutorial, let alone one some poor schmucks actually pay for. I say "possibly", as there's a perfectly good way of circumventing all of these subtle little bugs:
inline void assert (bool x, const char *file, int line) {
#ifdef DEBUG
if (!x) {
cerr << "ASSERT failed at " << file << " line " << line << endl;
abort();
}
#end
}
#define ASSERT(x) assert((x), __FILE__, __LINE__)Any good optimising compiler should be able to disappear calls to empty subroutines; regardless, if it's correctly inlined, the performance penalty should be negligible even if the assert() call isn't completely eliminated.
To get the rest of the way up on my soapbox; IMO, all C tutorials introduce the preprocessor too soon. Most uses of it really should be replaced with either applications of const, typedef, or (inlined) functions. If you must use a #define, it's vital IMO that its expansion is the same "sort" of code as the macro appears to be; either an atomic constant or a function call, as appropriate. To do otherwise without good reason and vigorous documentation is the path to many subtle and obscure heisenbugs.
-
This is minor in comparison to the other WTFs, but using cerr and endl without the namespace prefix is a bit optimistic.
Uhm, what am I talking about, everyone (especially authors of "teach yourself C++"-type books) knows that you shall always put 'using namespace std;' right after those ugly #includes
-
@Sad Bug Killer said:
Uhm, what am I talking about, everyone (especially authors of "teach yourself C++"-type books) knows that you shall always put 'using namespace std;' right after those ugly #includes
Right! They didn't name the namespace 'STD' for nothing.
-
I found it! I found it!
They used #end instead of #endif which means that whatever lies below these lines won't be run except when DEBUG is defined.
