523417 - "warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98" in DOM worker code

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Jeff Walden [:Waldo]]

DOM worker code seems to use this:

  NS_ENSURE_SUCCESS(rv,)

inside some constructors as a way to error-check, assuming that providing an empty parameter expands to nothing (or at best, whitespace).  This apparently isn't language-kosher, so I get these warnings (and many others in other files) when compiling:

/home/jwalden/moz/2/dom/src/threads/nsDOMThreadService.cpp:599:26: warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98
/home/jwalden/moz/2/dom/src/threads/nsDOMThreadService.cpp:599:26: warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro arguments are undefined in ISO C90 and ISO C++98

It looks like these should just be expanded to avoid using the macro incorrectly:

  if (NS_FAILED(rv))
    return;

Comment 1

[Jeff Walden [:Waldo]]

[jwalden@the-great-waldo-search js-tm]$ grep -r 'NS_ENSURE_SUCCESS\(.*,[[:space:]]*\);' .
./dom/src/threads/nsDOMWorkerTimeout.cpp:  NS_ENSURE_SUCCESS(*aRv,);
./dom/src/threads/nsDOMWorkerTimeout.cpp:  NS_ENSURE_SUCCESS(*aRv,);
./dom/src/threads/nsDOMWorkerTimeout.cpp:  NS_ENSURE_SUCCESS(*aRv,);
./dom/src/threads/nsDOMThreadService.cpp:    NS_ENSURE_SUCCESS(rv,);
./dom/src/threads/nsDOMThreadService.cpp:  NS_ENSURE_SUCCESS(rv,);
./dom/src/threads/nsDOMThreadService.cpp:  NS_ENSURE_SUCCESS(rv,);
./accessible/src/base/nsDocAccessible.cpp:    NS_ENSURE_SUCCESS(rv,);
./content/smil/nsSMILAnimationFunction.cpp:    NS_ENSURE_SUCCESS(InterpolateResult(values, result, aResult),);
./content/smil/nsSMILAnimationFunction.cpp:    NS_ENSURE_SUCCESS(AccumulateResult(values, result),);
./xpcom/tests/TestTimers.cpp:    NS_ENSURE_SUCCESS(rv,);
./xpcom/tests/TestProxies.cpp:    NS_ENSURE_SUCCESS(rv,);
./xpcom/tests/TestProxies.cpp:    NS_ENSURE_SUCCESS(rv,);

Unless my pattern is messed up (entirely possible, grep patterns, shell syntax, and I don't get along well), that's a complete list.

Comment 2

[Saint Wesonga]

Attachment: Patch

Comment 3

[Saint Wesonga]

Attachment: Patch for checkin

Applies cleanly

Comment 4

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/d80b2bb478dc

Comment 5

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

Why not use:
  NS_ENSURE_SUCCESS(rv, /**/);
instead, so we don't lose the NS_WARN_IF_FALSE?

Comment 6

[Jeff Walden [:Waldo]]

Does whitespace plus comment equal non-empty macro argument?  (For some reason my compiler is no longer warning on the stupid testcase I wrote to test this, which either means I'm not writing it correctly or [I suspect more likely] a compiler update took away the warning.)