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

RESOLVED FIXED in mozilla7

Status

()

Core
XSLT
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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?
Component: Style System (CSS) → XSLT
QA Contact: style-system → xslt
Oops.  You're correct that the desired grouping is:

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

Want to write a patch?  ;)
However, note that this code was not added in bug 84582.  It was added in bug 248258.
Blocks: 248258
No longer blocks: 84582
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
Created attachment 537962 [details] [diff] [review]
fix

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

Sure! Attached. :)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 5

6 years ago
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.
Attachment #537962 - Flags: review?(bzbarsky)
Comment on attachment 537962 [details] [diff] [review]
fix

r=me
Attachment #537962 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

6 years ago
http://hg.mozilla.org/mozilla-central/rev/0da6a98044d5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.