Last Comment Bug 737678 - Fix android-on-mac bustage caused by -Werror=declaration-after-statement
: Fix android-on-mac bustage caused by -Werror=declaration-after-statement
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on: 711895
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-20 15:59 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2012-03-21 22:19 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.80 KB, patch)
2012-03-20 16:08 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
jwalden+bmo: review+
Details | Diff | Review
bug-737678-fix-declaration-after-statement-warnings.patch (13.63 KB, patch)
2012-03-20 17:27 PDT, Chris Peterson [:cpeterson]
n.nethercote: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2012-03-20 15:59:55 PDT
+++ This bug was initially created as a clone of Bug #711895 +++

Bug 711895 changed -Wdeclaration-after-statement (which is only relevant for C, not C++) to -Werror=declaration-after-statement.  The rationale was that MSVC gives errors if declarations come after statement, so we might as well make all platforms behave the same.

However, some third-party, Android-specific code is triggering the warning when built on Mac.  Making the warning fatal doesn't gain us much, so the easiest thing is to just make the warning non-fatal.
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-03-20 16:08:12 PDT
Created attachment 607757 [details] [diff] [review]
patch

Trivial patch.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-20 16:11:57 PDT
Comment on attachment 607757 [details] [diff] [review]
patch

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

We should get this fixed upstream, then reenable this Werror=.  But for now this works.
Comment 3 Gregory Szorc [:gps] 2012-03-20 16:13:48 PDT
Can this flag get changed only in the areas of the tree that are broken? Why should the whole tree be subject to reduced checking because of "blessed violations" in a tiny sub component?
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-03-20 16:14:19 PDT
It appears that we make local changes to android/ all the time without worrying about upstream.  So modifying the code could also fix this, though there might be other similar cases lurking in the woods.  Any preferences?  If no-one speaks up, I'll land the r+'d patch later today.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-03-20 16:15:30 PDT
(In reply to Gregory Szorc [:gps] from comment #3)
> Can this flag get changed only in the areas of the tree that are broken?

I don't want to do that -- too complicated for minimal benefit.

> Why
> should the whole tree be subject to reduced checking because of "blessed
> violations" in a tiny sub component?

It's not a particularly important warning.  I just want to fix the bustage so I can put 711895 behind me.
Comment 6 Chris Peterson [:cpeterson] 2012-03-20 16:32:58 PDT
Here is a list of the Android files broken by -Werror=declaration-after-statement:

* ipc/chromium/src/third_party/libevent/event.c (already fixed upstream)
* js/src/ctypes/libffi/src/arm/ffi.c
* media/libsydneyaudio/src/sydney_audio_android.c (Mozilla code)
* other-licenses/android/getaddrinfo.c (Mozilla fork of Android code)

I am testing a preliminary fix for these files.
Comment 7 Chris Peterson [:cpeterson] 2012-03-20 17:27:28 PDT
Created attachment 607784 [details] [diff] [review]
bug-737678-fix-declaration-after-statement-warnings.patch

Fix -Werror=declaration-after-statement warnings on Android.

Also fix a sydney_audio_android.c warning about pointer constness and signedness (unsigned char* vs const jbyte*).
Comment 8 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-03-20 17:45:46 PDT
Comment on attachment 607784 [details] [diff] [review]
bug-737678-fix-declaration-after-statement-warnings.patch

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

Having to put declarations before statements really uglifies the code :(

Thanks for doing this.  I assume you're happy to land the patch?
Comment 9 Chris Peterson [:cpeterson] 2012-03-20 20:10:38 PDT
njn, I can't land the patch because I don't have L3 commit access yet. Also, I have not been able to run my patch through the try servers because they are down.
Comment 10 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-03-20 20:19:01 PDT
(In reply to Chris Peterson (:cpeterson) from comment #9)
> njn, I can't land the patch because I don't have L3 commit access yet. Also,
> I have not been able to run my patch through the try servers because they
> are down.

Ok, let me know once you've done a try run and then I'm happy to land the patch for you.  Thanks.
Comment 11 Mike Hommey [:glandium] 2012-03-20 23:37:31 PDT
(In reply to Nicholas Nethercote [:njn] from comment #4)
> It appears that we make local changes to android/ all the time without
> worrying about upstream.

I'm not convinced we do. We used to have non android code in there, and these moved. We also have the custom linker, which received a lot of changes, and for which we don't care about upstreaming because it doesn't make sense to. But the rest should mostly be untouched. For the few we touch there, if we don't upstream, that's bad imho.
Comment 12 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-03-21 01:52:21 PDT
Thinking about this some more, I think we should leave the android code alone and change the warning.  Rationale:

- Declarations after statements are good!  Disallowing them makes code much uglier.

- Declarations after statements are legal in C99, which is now almost 13 years old.  While Mozilla code may not be compiled under that standard by default, third-party code may be, and if our compilers handle it there doesn't seem to be much point in disallowing it.

- The only other GCC warning that we always force to be an error is -Werror=return-type, and that's because it's pretty much *always* indicative of a real defect.  In contrast, declarations after statements are *never* indicative of a real defect, assuming that the compiler handles them.

- -Wdeclaration-after-statement isn't even included in -Wall.

If anything, this is an argument for not using -Wdeclaration-after-statement at all.  But I suspect some people won't like that, so I think we should just make it non-fatal, as per my original patch.
Comment 13 Chris Peterson [:cpeterson] 2012-03-21 10:41:16 PDT
Sounds good to me. The files my patch changed were Android-only and not compiled by MSVC's C90 compiler.
Comment 14 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-03-21 22:19:51 PDT
Bug 711895 was backed out due to i10n bustage.  I'll fix this problem there when I reland.

Note You need to log in before you can comment on or make changes to this bug.