Closed Bug 827961 Opened 7 years ago Closed 7 years ago

nsHTMLMediaElement.cpp:3576:24: error: suggest parentheses around ‘&&’ within ‘||’ [-Werror=parentheses]

Categories

(Core :: Audio/Video, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox17 --- unaffected
firefox18 --- unaffected
firefox19 --- unaffected
firefox20 --- unaffected
firefox21 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

New build warning, treated as error in enable-warnings-as-error builds with newish GCC versions (I'm using ver 4.7 on Ubuntu 12.10):

{
content/html/content/src/nsHTMLMediaElement.cpp: In member function ‘void nsHTMLMediaElement::UpdateAudioChannelPlayingState()’:

content/html/content/src/nsHTMLMediaElement.cpp:3576:24: error: suggest parentheses around ‘&&’ within ‘||’ [-Werror=parentheses]
}
(encountered this while building w/ --enable-application=b2g to make B2G desktop client, FWIW)
The obvious fix is to add the (already implicit) parenthesis that the warning suggests.

I haven't dug into the logic here to see if this grouping is what's really intended -- Marco, does this grouping match what you were going for in bug 819852?
Attachment #699368 - Flags: review?(mchen)
(In reply to Daniel Holbert [:dholbert] from comment #2)
> I haven't dug into the logic here to see if this grouping is what's really
> intended

From staring at this slightly more, I think this makes sense.

It looks like this code says we're playing audio IF:
 (1) we're not paused, AND EITHER:
   (a) we're looping (in which case we're playing forever, I guess)
   ...OR...
   (b) we have data AND we haven't reached the end of it yet.

And this bug's patch just adds explicit parens around the conditions in line (b) there.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Comment on attachment 699368 [details] [diff] [review]
fix: add the already-implicit parenthesis

RyanVM told me this manifested as a build error on B2G builds in the b2g18 branch when bug 819852's patch landed there.
https://tbpl.mozilla.org/php/getParsedLog.php?id=18606563&tree=Mozilla-B2g18
(Not sure why that didn't happen on m-c -- we must have a different build config there, or maybe the comparable build is hidden or something.)

I suspect mchen might be asleep -- roc, perhaps you could review this, since you reviewed bug 819852?
Attachment #699368 - Flags: review?(roc)
(In reply to Daniel Holbert [:dholbert] from comment #4)
> (Not sure why that didn't happen on m-c -- we must have a different build
> config there, or maybe the comparable build is hidden or something.)

(ah, looks like the failure is for a desktop b2g build, and I don't think we have any of those for m-c TBPL. That's why this didn't cause any issues on m-c.)
Comment on attachment 699368 [details] [diff] [review]
fix: add the already-implicit parenthesis

Review of attachment 699368 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely correct
Attachment #699368 - Flags: review?(roc)
Attachment #699368 - Flags: review?(mchen)
Attachment #699368 - Flags: review+
Landed on mozilla-b2g18 with a=bustage (didn't want to wait on release drivers, or eat up any of their time, since this fixes a currently-broken build on TPBL and it's a near-NPOTB change anyway (just adding parens that are already implied):
  https://hg.mozilla.org/releases/mozilla-b2g18/rev/81910ad56238
OS: Linux → All
Dear Daniel and Robert,

Thanks for your help here.
https://hg.mozilla.org/mozilla-central/rev/f48ed62a1dee
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.