260209 - Debug macros (NS_ASSERTION etc.) not followed by a ';' should produce a compiler error also in non-debug builds

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Hans-Andreas Engel]

The following code does not compile if DEBUG is defined, since there is a
missing semicolon after the second line.

1    foo();
2    NS_ASSERTION(1, "assertion always holds")
3    foo();

However, somewhat inconsistently, the code compiles fine if DEBUG is not
defined.  The expected behavior is that the compiler reports an error.

Comment 1

[Hans-Andreas Engel]

The definition of NS_ASSERTION is in
 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/glue/nsDebug.h&rev=1.13#201

#ifndef DEBUG
(...)
208     #define NS_ASSERTION(expr, str)        /* nothing */

Thus, in a non-debug build a semicolon is not enforced after |NS_ASSERTION(..)|.

Comment 2

[Hans-Andreas Engel]

To be more consistent with the debug-version, the following macro definition
could be used, and equivalently for the other macros.

#define NS_ASSERTION(expr, str)   PR_BEGIN_MACRO  /* nothing */  PR_END_MACRO

(This results in  |do{/*nothing*/} while(0)|  and the compiler complains if
|NS_ASSERTION()| is not followed by a semicolon.)

Comment 3

[Hans-Andreas Engel]

Attachment: suggested fix; defines empty macros as |PR_BEGIN_MACRO /* nothing */ PR_END_MACRO|

Comment 4

[Doug Turner (:dougt)]

is this platform specific?  These seams to work:

http://lxr.mozilla.org/seamonkey/search?string=NS_ASSERTION%281%2C

Comment 5

[Hans-Andreas Engel]

Comment 4 gives the following code as example.
   NS_ASSERTION(1, "possible plugin performance issue");

If you remove the semicolon, namely use
   NS_ASSERTION(1, "possible plugin performance issue")
then
i)  #ifdef  DEBUG  --> you get a compiler error,
ii) #ifndef DEBUG  --> no compiler error.

In my opinion, the compiler should report an error in both cases.

I think this is not platform specific.

Comment 6

[Hans-Andreas Engel]

...unless, of course, if one uses a compiler (are there any?) which tolerate a
missing semicolon after a do-while statement, such as
  void f() {
    do{ } while(0)
  }

Here, gcc gives a "parse error before `}' token."  For Visual C++ a ';' is
required as well (at least in the language definition),
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vclang/html/_pluslang_the_do_statement.asp

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 159323 [details] [diff] [review]
suggested fix; defines empty macros as |PR_BEGIN_MACRO /* nothing */ PR_END_MACRO|

thanks. I can check this in, or would you like to?

Comment 8

[Hans-Andreas Engel]

Yes, please check it in for me.  I do not have CVS write access.

Would this require a super-review first?

Comment 9

[Benjamin Smedberg]

Doug, I'm not certain this is a good idea. Will it affect optimizations? I know
that compilers are supposed to be smart enough to remove empty loops and such,
but if we really want to make this change we should watch the codesize and perf
numbers very carefully.

Comment 10

[Hans-Andreas Engel]

Yes, this patch assumes that the compiler optimizes |do{}while(0);| away.

gcc on cygwin optimizes this, even without any "-O*" flag.

I perfectly agree that one should keep an eye on codesize and performance (as
one probably should for every patch), but I would be surprised to learn that
some compilers would not optimize this.

Comment 11

[Henry Jia]

Comment on attachment 159323 [details] [diff] [review]
suggested fix; defines empty macros as |PR_BEGIN_MACRO /* nothing */ PR_END_MACRO|

We need to keep the compile result consistent. Other than that, we can't expect
developers to build applications easily until they are quite familiar with the
optimation details instead of using syntax.

sr=Henry

Also to see if there are any other opinions.

Comment 12

[Doug Turner (:dougt)]

Checking in nsDebug.h;
/cvsroot/mozilla/xpcom/glue/nsDebug.h,v  <--  nsDebug.h
new revision: 1.15; previous revision: 1.14
done


Thank for the patch!  Lets keep an eye on the tb numbers, if anything changes to
opt numbers, we will back this out.