Closed
Bug 1236430
Opened 9 years ago
Closed 9 years ago
Try to enable -Werror=return-type by default
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: jerry, Assigned: jerry)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
cpeterson
:
feedback-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Comment hidden (obsolete) |
Assignee | ||
Comment 3•9 years ago
|
||
add the same flag into CXXFLAGS.
Assignee | ||
Comment 4•9 years ago
|
||
Comment 5•9 years ago
|
||
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.
Assignee | ||
Comment 6•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
Thx, I will try that again after bug 1232224.
Assignee | ||
Updated•9 years ago
|
Attachment #8703512 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8703516 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
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.
Attachment #8704011 -
Flags: review?(n.nethercote)
Attachment #8704011 -
Flags: review?(cpeterson)
Comment 11•9 years ago
|
||
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.
Attachment #8704011 -
Flags: review?(n.nethercote)
Comment 12•9 years ago
|
||
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.
Attachment #8704011 -
Flags: review?(cpeterson) → feedback-
Comment 13•9 years ago
|
||
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.
Assignee | ||
Comment 14•9 years ago
|
||
Thx, close this bug.
Use --enable-warnings-as-errors in mozconfig instead.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•