120899 - Current impl of NS_WARN_IF_FALSE() considerd harmful

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Johnny Stenback (:jst)]

Currently NS_WARN_IF_FALSE() is written to "return" the value of the expression
to be useable in expressions. This fact is somewhat nice (although not much),
but it's also harmful and mis-useable. This fact means that NS_WARN_IF_FALSE()
does *not* expand to nothing in release builds. IOW if you do:

  NS_WARN_IF_FALSE(anobject.some_expensive_operation(), "balh");

this can give useful debugging information, but the above line will end up
executing anobject.some_expensive_operation() in optimized builds for no good
reason. I realized this when I was stepping through the code in
nsHTMLDocument::GetElementById() at:

http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLDocument.cpp#2594

in an optimized-with-symbols build and I saw us step through
aElementId.IsEmpty() twice. I bet there's more of this around that cause code
that was intended to be debug only to be executed in optimized builds.

IMO we have two choices, either fix the users of this macro, or change the macro
so that you can't cause unexpected things to happen in optimized builds with it.
I vote for the latter. Patch coming up that does exactly that.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Change NS_WARN_IF_FALSE() to expand to nothing in non-debug code.

Comment 2

[Johnny Stenback (:jst)]

Thoughts?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This inconsistency always bugged me.  It's also annoying because of the numerous
build warnings it causes in optimized builds.

Perhaps you should drop the parentheses in the DEBUG version to be consistent
with the other macros in nsDebug.h?  Other than that, r=dbaron.

Comment 4

[Johnny Stenback (:jst)]

Attachment: Updated patch for nsDebug only that contains dbaron's suggestion and fixes some other inconsistencies.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 65775 [details] [diff] [review]
Updated patch for nsDebug only that contains dbaron's suggestion and fixes some other inconsistencies.

>-#define NS_WARN_IF_FALSE(_e,_msg) \
>-  (!(_e) ? nsDebug::WarnIfFalse(_msg, #_e, __FILE__, __LINE__) : PR_FALSE)
>+#define NS_WARN_IF_FALSE(_expr,_msg)                         \
>+  if (!(_expr)) {                                            \
>+    nsDebug::WarnIfFalse(_msg, #_e, __FILE__, __LINE__)      \
>+  }                                                          \

(1) Don't invite dangling else problems -- use PR_BEGIN_MACRO and PR_END_MACRO,
or use a (?:) expression
(2) Don't put a backslash at the end of the last line of a multiline macro.

>-#define NS_WARN_IF_FALSE(_e,_msg) \
>-  (!(_e) ? PR_TRUE : PR_FALSE)
>+#define NS_WARN_IF_FALSE(_expr,_msg) {}

This too is problematic, in an unbraced then statement it requires no
semicolon, or else bracing.  Just define useless macros like this as ((void)0)
or /* nothing */.

> 
> #define NS_PRECONDITION(expr,str)  {}
> #define NS_ASSERTION(expr,str)     {}

History: NS_WARN_IF_FALSE was meant to expand in release builds so that it
could be used as an if condition, but obviously, it got muddled along the way
(both in the comments here, and in its usage).	That suggests it was a bad idea
to combine a useful expression result with a (non-debug) useless warning test.

/be

Comment 6

[Johnny Stenback (:jst)]

Attachment: New patch for nsDebug.h including brendan's suggestions.

Comment 7

[Johnny Stenback (:jst)]

Attachment: This part was missing from the first patch (nsTraceRefcnt.cpp changes)

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 65807 [details] [diff] [review]
This part was missing from the first patch (nsTraceRefcnt.cpp changes)

sr=[e-mail]

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 65806 [details] [diff] [review]
New patch for nsDebug.h including brendan's suggestions.

>-#define NS_WARN_IF_FALSE(_e,_msg) \
>-  (!(_e) ? nsDebug::WarnIfFalse(_msg, #_e, __FILE__, __LINE__) : PR_FALSE)
>+#define NS_WARN_IF_FALSE(_expr,_msg)                         \
>+PR_BEGIN_MACRO                                               \
>+  if (!(_expr)) {                                            \
>+    nsDebug::WarnIfFalse(_msg, #_expr, __FILE__, __LINE__);  \
>+  }                                                          \
>+PR_END_MACRO

Nit: I usually indent one level for any macro guts in a multiline definition.

>-#define NS_WARN_IF_FALSE(_e,_msg) \
>-  (!(_e) ? PR_TRUE : PR_FALSE)
>+#define NS_WARN_IF_FALSE(_expr,_msg) /* nothing */

Cool, but:

> #define NS_PRECONDITION(expr,str)  {}
> #define NS_ASSERTION(expr,str)     {}

Fix these also?

>@@ -327,8 +314,10 @@
> 
> #define NS_ENSURE_TRUE(x, ret)                                                \
>   PR_BEGIN_MACRO                                                              \
>-   if(NS_WARN_IF_FALSE(x, "NS_ENSURE_TRUE(" #x ") failed"))                   \
>+   if(!(x)) {                                                                 \
>+     NS_WARNING("NS_ENSURE_TRUE(" #x ") failed");                             \
>      return ret;                                                              \
>+  }                                                                           \
>   PR_END_MACRO

space after if should be prevailing style here, no?

Fix what nits you can, and sr=[e-mail].

/be

Comment 10

[Daniel Bratell]

IMHO, this seems a little too dangerous for 0.9.8 to me. What if there´s code
out there that depends on the execution of the warning condition even in
optimized builds? 

Comment 11

[Brendan Eich [:brendan]]

This is *not* slated for 0.9.8 -- no keyword nomination, no 115520 dependent
bug.  jst, please retarget at 0.9.9.

/be

Comment 12

[Robert Ginda]

I'm not sure why jsd_xpc.cpp needs these changes?  Isn't that the indended usage
of WARN_IF_FALSE?

Comment 13

[Robert Ginda]

Sorry, I guess I need to pay more attention, I see now.  I kinda liked
NS_WARN_IF_FALSE, but I'll get over it.  r=rginda on the jsd changes.

Comment 14

[Johnny Stenback (:jst)]

Um, I never intended this to be targetted at mozilla0.9.8, targetting for
mozilla0.9.9.

Comment 15

[Johnny Stenback (:jst)]

Attachment: Change users of NS_VERIFY() to use NS_ASSERTION() since the implementation of NS_VERIFY is as evil as NS_WARN_IF_FALSE()'s

Comment 16

[Johnny Stenback (:jst)]

Attachment: Fix nits and eliminate NS_VERIFY() from nsDebug.h

Comment 17

[Daniel Bratell]

in Windows programming there are two familys of assertion macros:

     ASSERT(expr)   that executes and asserts in debug builds when expr == 0
                    and does nothing when optimized and:

     VERIFY(expr)   that executes expr in all builds but also asserts in 
                    debug builds if expr == 0.

So people coming from the Windows world would expect the expression in NS_VERIFY
to always execute but combined with all the other assertion macros I can agree
that it might be confusing. There are alot of them!

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The current patch being discussed will break things like:

http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#1657

in non-DEBUG builds.

Comment 19

[Johnny Stenback (:jst)]

Attachment: Make the macro's expand to ((void0) in optimized builds.

Yeah, true.

Comment 20

[Brendan Eich [:brendan]]

I pointed out to dbaron that if (NS_FAILED(ret)) /* nothing */; is fine, the ;
makes an empty then statement.  There should be no difference between defining
NS_WARNING et al. as /* nothing */ and as ((void) 0).  Matter of taste, I prefer
/* nothing */.

/be

Comment 21

[Johnny Stenback (:jst)]

Oh so true. I also prefer /* nothing */, so I'll go with that for now...

Comment 22

[Johnny Stenback (:jst)]

Fixed.