1236430 - Try to enable -Werror=return-type by default

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I hit some crashes when I have a typo for my function return statement.
For desktop browser build, we don't turn on |--enable-warnings-as-errors|. There is no build error with this option for mac desktop browser build. So, I'm trying to turn on the |-Werror=return-type| option by default.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Enable -Werror=return-type by default. v1

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=700a7296a631

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Enable -Werror=return-type by default. v2

add the same flag into CXXFLAGS.

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try for attachment 8703516 [details] [diff] [review]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d247ea52ee5

Comment 5

[Nicholas Nethercote [inactive]]

The patch in bug 1232224 landed today and so the attached patch no longer applies.

More generally, I don't think this is worthwhile. For local builds --enable-warnings-as-errors is not used, but it *is* used for all builds visible on Treeherder. So it's impossible to land code with bad returns without getting notified.

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

If we already turn on |--enable-warnings-as-errors| for all try build, why we don't add that flag for local build?
And this flag help me to check my coding mistake at the beginning.

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #6)
> If we already turn on |--enable-warnings-as-errors| for all try build, why
> we don't add that flag for local build?

You can't have it on by default because it breaks the build for people using uncommon compilers (e.g. dev versions of GCC and clang, which often add new warnings). We've tried it in the past and it caused too many problems.

Comment 8

[Chris Peterson [:cpeterson]]

We used to enable -Werror=return-type by default, but it got lost in some configure.in refactoring. If you want to make this change, don't forget to also update js/src/configure.in.

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Thx, I will try that again after bug 1232224.

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Enable -Werror=return-type by default.

Wait for try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=365d93c04ce0

I'm not sure the whole story in bug 1232224, but there is no build error for b2g and desktop brower at MAC 10.11(xcode 7.2) with the return-type checking.

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8704011 [details] [diff] [review]
Enable -Werror=return-type by default.

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

The code is correct, but I don't think the patch is necessary for the reasons explained in comment 4.

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 8704011 [details] [diff] [review]
Enable -Werror=return-type by default.

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

I'm not a build config peer, so I can't r+ this patch. While this error might be nice to have, I agree with Nicholas that it's not necessary. Treeherder will catch -Wreturn-type bugs before they land in mozilla-central. Developers can catch them locally by adding `ac_add_options --enable-warnings-as-errors` to their local mozconfig.

We've backed away from forcing -Werror for some other warning flags, too. So warnings really are just warnings unless you opt in with --enable-warnings-as-errors.

Comment 13

[Nicholas Nethercote [inactive]]

Yes, if you use --enable-warnings-as-errors in your local build your compiler will catch this problem and various others. You should do that.

Comment 14

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Thx, close this bug.
Use --enable-warnings-as-errors in mozconfig instead.