Last Comment Bug 662685 - txMozillaXMLOutput.cpp:1082:66: warning: suggest parentheses around ‘&&’ within ‘||’
: txMozillaXMLOutput.cpp:1082:66: warning: suggest parentheses around ‘&&’ with...
Status: RESOLVED FIXED
[build_warning]
:
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Daniel Holbert [:dholbert]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 248258
  Show dependency treegraph
 
Reported: 2011-06-07 17:05 PDT by Daniel Holbert [:dholbert]
Modified: 2011-06-09 12:17 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (1.20 KB, patch)
2011-06-08 00:20 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-06-07 17:05:52 PDT
Filing bug on this warning:
{
content/xslt/src/xslt/txMozillaXMLOutput.cpp: In member function ‘void txTransformNotifier::SignalTransformEnd(nsresult)’:
content/xslt/src/xslt/txMozillaXMLOutput.cpp:1082:66: warning: suggest parentheses around ‘&&’ within ‘||’
}

The flagged code is:
> 1078 void
> 1079 txTransformNotifier::SignalTransformEnd(nsresult aResult)
> 1080 {
> 1081     if (mInTransform || (NS_SUCCEEDED(aResult) &&
> 1082         mScriptElements.Count() > 0 || mPendingStylesheetCount > 0)) {
> 1083         return;
> 1084     }
http://mxr.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txMozillaXMLOutput.cpp#1078

In particular, it's suggesting that we add an extra layer of parens like so
(adding whitespace to clarify the groupings):
>          if (mInTransform ||
>              ((NS_SUCCEEDED(aResult) && mScriptElements.Count() > 0) ||
>               mPendingStylesheetCount > 0)) {
>              return;

(The above is equivalent to what we already have)

However, given the structure of this code right now, I suspect that the intention here might be different from what we actually get.

It looks like we might *intend* for there to be parens around the final two conditions, like so:
> 1081     if (mInTransform || (NS_SUCCEEDED(aResult) &&
> 1082         (mScriptElements.Count() > 0 || mPendingStylesheetCount > 0))) {
> 1083         return;
> 1084     }
...which is different from the behavior we currently have.

bz, do you remember which condition-grouping we wanted here?
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-06-07 22:19:16 PDT
Oops.  You're correct that the desired grouping is:

  NS_SUCCEEDED(aResult) &&
    (mScriptElements.Count() > 0 || mPendingStylesheetCount > 0)

Want to write a patch?  ;)
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-06-07 22:22:45 PDT
However, note that this code was not added in bug 84582.  It was added in bug 248258.
Comment 3 Daniel Holbert [:dholbert] 2011-06-08 00:13:32 PDT
(In reply to comment #2)
> However, note that this code was not added in bug 84582.  It was added in
> bug 248258.

Ah, right; bug 84582 was just the last bug to have touched that line (apparently just doing a variable-rename).  Sorry for the mis-attribution.
Comment 4 Daniel Holbert [:dholbert] 2011-06-08 00:20:50 PDT
Created attachment 537962 [details] [diff] [review]
fix

(In reply to comment #1)
> Want to write a patch?  ;)

Sure! Attached. :)
Comment 5 Daniel Holbert [:dholbert] 2011-06-08 00:22:50 PDT
Comment on attachment 537962 [details] [diff] [review]
fix

Patch just adds parens around the two "count" checks.

Patch also adds newline before the NS_SUCCEEDED check, for clarity & to save on having to hugely indent everything after it.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-06-08 10:43:25 PDT
Comment on attachment 537962 [details] [diff] [review]
fix

r=me
Comment 7 Daniel Holbert [:dholbert] 2011-06-09 12:17:42 PDT
http://hg.mozilla.org/mozilla-central/rev/0da6a98044d5

Note You need to log in before you can comment on or make changes to this bug.