662685 - txMozillaXMLOutput.cpp:1082:66: warning: suggest parentheses around ‘&&’ within ‘||’

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Daniel Holbert [:dholbert]]

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 [:bzbarsky]]

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 [:bzbarsky]]

However, note that this code was not added in bug 84582.  It was added in bug 248258.

Comment 3

[Daniel Holbert [:dholbert]]

(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]]

Attachment: fix

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

Sure! Attached. :)

Comment 5

[Daniel Holbert [:dholbert]]

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 [:bzbarsky]]

Comment on attachment 537962 [details] [diff] [review]
fix

r=me

Comment 7

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/rev/0da6a98044d5