Closed
Bug 666672
Opened 13 years ago
Closed 13 years ago
media/ - compiler warnings on mac
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: joey, Assigned: atulagrwl)
References
(Blocks 1 open bug)
Details
(Whiteboard: [build_warning])
Attachments
(1 file, 4 obsolete files)
18.95 KB,
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
% uname -a Darwin banshee.local 10.7.4 Darwin Kernel Version 10.7.4: Mon Apr 18 21:24:17 PDT 2011; root:xnu-1504.14.12~3/RELEASE_X86_64 x86_64 /mozilla/sandbox/gml/media/libvorbis/lib/vorbis_codebook.c:251: warning: suggest parentheses around + or - inside shift /mozilla/sandbox/gml/media/libvorbis/lib/vorbis_floor1.c:1038: warning: suggest parentheses around + or - in operand of & /mozilla/sandbox/gml/media/libvorbis/lib/vorbis_psy.c:1164:17: warning: "/*" within comment /mozilla/sandbox/gml/media/libtheora/lib/state.c:717: warning: comparison of unsigned expression < 0 is always false /mozilla/sandbox/gml/media/libtheora/lib/state.c:718: warning: comparison of unsigned expression < 0 is always false /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:141: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:183: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:190: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:202: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:225: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:276: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:328: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:375: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:433: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:437: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:449: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:477: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:518: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:616: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:627: warning: ISO C90 forbids mixed declarations and code /mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:664: warning: ISO C90 forbids mixed declarations and code
Reporter | ||
Updated•13 years ago
|
Whiteboard: [build_warnings]
Updated•13 years ago
|
Whiteboard: [build_warnings] → [build_warning]
Updated•13 years ago
|
Blocks: buildwarning
Assignee | ||
Comment 1•13 years ago
|
||
Fixed all the issues except "warning: comparison of unsigned expression < 0 is always false". The reason being the check is there to ensure enum is within range by enum <0 and enum > MAX_LIMIT but gcc might have implemented enum as unsigned int and hence it is throwing warning that one check is always false but this might not be the case with other compilers and we don't want to miss an important check.
Assignee: nobody → atulagrwl
Status: NEW → ASSIGNED
Comment 2•13 years ago
|
||
One of the libvorbis issues are fixed slightly differently upstream. I suggest copying that patch to reduce noise at the next merge. https://trac.xiph.org/changeset/17688 The code in question is #if 0'd out, so the uncommenting the related code isn't a functional change.
Assignee | ||
Comment 3•13 years ago
|
||
Accommodating the changes done upstream to fix the warning.
Attachment #556993 -
Attachment is obsolete: true
Attachment #557030 -
Flags: review?(giles)
Comment 4•13 years ago
|
||
Comment on attachment 557030 [details] [diff] [review] Patch v2 Thanks. Looks good. My only comment is that it would suit local style better if in the sydneyaudio code when you add a set of declarations at the start of a function, you follow it with a blank line, like in audio_callback(). Also, why doesn't sa_stream_get_volume_abs() warn about local_vol around sydney_audio_mac.c:677?
Comment 5•13 years ago
|
||
Comment on attachment 557030 [details] [diff] [review] Patch v2 Drive by comments. >diff --git a/media/libsydneyaudio/src/sydney_audio_mac.c b/media/libsydneyaudio/src/sydney_audio_mac.c >--- a/media/libsydneyaudio/src/sydney_audio_mac.c >+++ b/media/libsydneyaudio/src/sydney_audio_mac.c > >+ sa_stream_t * s; ... >- sa_stream_t * s = arg; >+ *s = arg; This dereferences an uninitialized variable, s. The "*s = arg" is not the same as "sa_stream_t* s = arg". + *dst = data->mBuffers[0].mData; As above. 'dst' is uninitialized. Should be 'dst ='. And here: - sa_buf * next = s->bl_head->next; + *next = s->bl_head->next; 'next' is uninitialized. >+ struct timespec ts; > pthread_mutex_unlock(&s->mutex); >- struct timespec ts = {0, 1000000}; >+ ts = {0, 1000000}; This can be: >+ struct timespec ts = {0, 1000000}; > pthread_mutex_unlock(&s->mutex); >- struct timespec ts = {0, 1000000}; There are a few places where you've moved code like this: Foo* a = 0; To be: Foo* a; ... a = 0; Since you are assigning a constant you could just move the variable declaration and still initialize it in one. For the vorbis changes you'll need to add a patch file, update the README_MOZILLA, and change update.sh to apply the patch.
Assignee | ||
Comment 6•13 years ago
|
||
Ralph, I don't have mac system and hence have relied upon the warning reported by the bug and fixed them. Due to this, I could not figure out the problem which Chris reported. Thanks a lot Chris for reporting this issue. I am feeling kind of stupid by committing blunder. Maybe I was half asleep when I prepared this patch. I will prepare another patch.
Comment 7•13 years ago
|
||
Ah, you should have said it was untested; I could have tried it. Thanks, Chris, for catching the spurious dereferences.
Assignee | ||
Comment 8•13 years ago
|
||
I have tried to fix all the issue raised. Please see if there is still some problem with it. Please note this is untested. Also I think there is some problem in the update.sh file of libsydneyaudio. Initial lines of patches has -p4 argument which I guess needs to be corrected to -p3 (Maybe?).
Attachment #557030 -
Attachment is obsolete: true
Attachment #557100 -
Flags: review?(giles)
Attachment #557100 -
Flags: review?(chris.double)
Attachment #557030 -
Flags: review?(giles)
Assignee | ||
Comment 9•13 years ago
|
||
Removed some whitespaces.
Attachment #557100 -
Attachment is obsolete: true
Attachment #557325 -
Flags: review?(giles)
Attachment #557325 -
Flags: review?(chris.double)
Attachment #557100 -
Flags: review?(giles)
Attachment #557100 -
Flags: review?(chris.double)
Comment 10•13 years ago
|
||
> ts = {0, 1000000};
This doesn't compile. The initializer syntax is only allowed on declarations. Despite making it harder to read, it's probably best to just move the initializer up to the declaration as well. Otherwise you need:
struct timespec ts;
...
ts.tv_sec = 0;
ts.tv_nsec = 1000000;
nanosleep(&ts, NULL);
which I don't think is any better.
Comment 11•13 years ago
|
||
Comment on attachment 557325 [details] [diff] [review] Patch v4 Review of attachment 557325 [details] [diff] [review]: ----------------------------------------------------------------- BTW, I think you're right about the -p4 lines, but don't worry about fixing it as part of this bug. It will be obvious the next time the code is sync'd with 'upstream'.
Comment 12•13 years ago
|
||
(In reply to Atul Aggarwal from comment #8) > Please note this is untested. Can you push to try server to make sure this issue is fixed on Mac? I don't have a mac to test with.
Comment 13•13 years ago
|
||
There's no need to include bug666672_gccWarnings.patch for the sydneyaudio patches. We stopped doing this some time ago, and the patches, update.sh, and readmes that remain in media/libsydneyaudio should be removed or updated to reflect the current practices.
Comment 14•13 years ago
|
||
I think it's worth maintaining the patch list while the code in tree is configured that way. If you want to remove the update mechanism, that should be an independent bug.
Comment 15•13 years ago
|
||
After further discussion, I now agree with Matthew. Please remove the the separate patch and changes to update.sh and README_MOZILLA for libsyndeyaudio. The separate patch file and update.sh changes for libvorbis should still be included.
Assignee | ||
Comment 16•13 years ago
|
||
Attachment #557325 -
Attachment is obsolete: true
Attachment #557413 -
Flags: review?(giles)
Attachment #557413 -
Flags: review?(chris.double)
Attachment #557325 -
Flags: review?(giles)
Attachment #557325 -
Flags: review?(chris.double)
Comment 17•13 years ago
|
||
Comment on attachment 557413 [details] [diff] [review] Patch v5 Review of attachment 557413 [details] [diff] [review]: ----------------------------------------------------------------- Compiles and plays sound on MacOS. Looks ok to me.
Attachment #557413 -
Flags: review?(giles) → review+
Comment 18•13 years ago
|
||
Build bustage on Mac OS X from try server: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=001b40a7bee3 http://tbpl.allizom.org/php/getParsedLog.php?id=6224676#error0
Comment 20•13 years ago
|
||
I'm told those links should be: http://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=001b40a7bee3 http://tbpl.mozilla.org/php/getParsedLog.php?id=6224676#error0 Chris, are you sure you pushed v5 of the patch? That looks like the compile problem I reported against v4 in comment 10.
Comment 21•13 years ago
|
||
Oops, you're right. New try server attempt: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ad4b478ce996
Assignee | ||
Comment 22•13 years ago
|
||
Are test results look okay?
Comment 23•13 years ago
|
||
Excellent question. The red tests look spurious. I've requested respins of those and the purple. Of the orange, most have intermittent bugs filed, and the rest look unrelated to me. Chris? Can you offer some guidance in reading the report?
Comment 24•13 years ago
|
||
With the exception of Win64, which is still pending, all the respins I requested came back greener. I think the tryserver run shows no new issues at this point.
Keywords: checkin-needed
Assignee | ||
Comment 26•13 years ago
|
||
Chris, can you please review the fix? Fix is quite simple and it won't take long. However, if you don't have time to review the fix, can you please add somebody appropriate to review it. Thanks
Comment 27•13 years ago
|
||
Comment on attachment 557413 [details] [diff] [review] Patch v5 If giles says it's fine, then that's review enough.
Attachment #557413 -
Flags: review?(chris.double)
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 28•13 years ago
|
||
Thanks, Chris.
Comment 29•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f61c120e00f2
Keywords: checkin-needed
Target Milestone: --- → mozilla9
Comment 30•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f61c120e00f2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•