737678 - Fix android-on-mac bustage caused by -Werror=declaration-after-statement

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nicholas Nethercote [inactive]]

+++ 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 [inactive]]

Attachment: patch

Trivial patch.

Comment 2

[Jeff Walden [:Waldo]]

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]]

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 [inactive]]

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 [inactive]]

(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]]

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]]

Attachment: 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 [inactive]]

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]]

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 [inactive]]

(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]]

(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 [inactive]]

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]]

Sounds good to me. The files my patch changed were Android-only and not compiled by MSVC's C90 compiler.

Comment 14

[Nicholas Nethercote [inactive]]

Bug 711895 was backed out due to i10n bustage.  I'll fix this problem there when I reland.