Closed Bug 577207 Opened 11 years ago Closed 11 years ago

vp8 warning: empty body in an if-statement

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: mkohler)

References

()

Details

Attachments

(1 file, 3 obsolete files)

media/libvpx/vp8/

decoder/decodemv.c: In function ‘vp8_decode_mode_mvs’:
decoder/decodemv.c:235: warning: empty body in an if-statement
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #456236 - Flags: review?(chris)
i've filed a bug for upstream:
http://code.google.com/p/webm/issues/detail?id=123
Assignee: timeless → michaelkohler
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #456236 - Attachment is obsolete: true
Attachment #456280 - Flags: review?(tterribe)
Attachment #456236 - Flags: review?(chris)
Attached patch Patch v3 (obsolete) — Splinter Review
.. sorry about this spam ..
Attachment #456280 - Attachment is obsolete: true
Attachment #456291 - Flags: review?(tterribe)
Attachment #456280 - Flags: review?(tterribe)
Comment on attachment 456291 [details] [diff] [review]
Patch v3

Personally I would be the entire if block inside #if CONFIG_DEBUG, but I don't think that's actually important.
Attachment #456291 - Flags: review?(tterribe) → review+
Could you please write a commit message for committing this to the upstream? Honestly, I have no idea what this patch is about, so I need your help.

BTW: timeless, do you want any credits for this bug in the commit message?
timeless@gmail.com is sufficient
err, wait...

timeless@mozdev.org for author in realm:*.mozilla.org

timeless@gmail.com for author if it's upstream
so, derf's right, the whole block is useless.

can you redo the patch as:

+                         while (j != L[++k])
++                        {
++#if CONFIG_DEBUG
+                             if (k >= 16)
++                            {
+-#if CONFIG_DEBUG
+                                 assert(0);
+-
+-#else
+-                                ;
++                            }
+ #endif
++                        }

the commit message should probably be something like:

limit range checking code for L[k] to CONFIG_DEBUG
Attached patch Patch v4Splinter Review
I will upload this patch to the libvpx repository later this evening.
Attachment #456291 - Attachment is obsolete: true
Attachment #456869 - Flags: review+
Still no verification upstream.

derf: could you get someone to do that? ;)
Upstream has been kicked and the patch merged.
(In reply to comment #13)
> Upstream has been kicked and the patch merged.

Thanks a lot.I don't think we want to wait till the next libvpx update, so I think we need to checkin that ourself.Should be a low risk checkin AFAIK.
Keywords: checkin-needed
Well, only blockers are being allowed until Fx 4, and I don't think warning fixes count as that.
(In reply to comment #15)
> Well, only blockers are being allowed until Fx 4, and I don't think warning
> fixes count as that.

Oh, right. Forgot about the discussion in moz.dev.planning [1].. But does this mean we just land blockers till Fx 4 and no new bug fixes that make a real difference and are low risk patches? Frankly, this doesn't seem very reasonable to me since the Fx 4 release is still a few month away. Therefore asking for approval-2.0.
Comment on attachment 456869 [details] [diff] [review]
Patch v4

this is a low risk change. I guess we could take it, but it's not my decision eventually.
Attachment #456869 - Flags: approval2.0?
Attachment #456869 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/a4b7ad2f4854
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.