Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-28 Thread Gregory Szorc
On 2/27/2014 2:02 PM, Nicholas Nethercote wrote: On Thu, Feb 27, 2014 at 12:44 PM, Zack Weinberg za...@panix.com wrote: Treating these as warnings, not errors, is probably the best thing here. If you see the warning and you've recently changed that code, then check it. If you haven't, you

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-28 Thread Boris Zbarsky
On 2/28/14 10:49 AM, Gregory Szorc wrote: Speaking of compiler warnings, do people commonly run into compiler warning mismatch with warnings-as-errors due to running separate versions of Clang/GCC/MSVC locally than what runs in automation? I did, to the point where I locally don't

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-28 Thread Ehsan Akhgari
On 2014-02-28, 11:06 AM, Boris Zbarsky wrote: On 2/28/14 10:49 AM, Gregory Szorc wrote: Speaking of compiler warnings, do people commonly run into compiler warning mismatch with warnings-as-errors due to running separate versions of Clang/GCC/MSVC locally than what runs in automation? I did,

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-27 Thread Zack Weinberg
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 02/23/2014 11:54 PM, Daniel Holbert wrote: On 02/22/2014 12:26 PM, Hubert Figuière wrote: FWIW, I (and others) have been working on that, as a side project, for a while now, and I think we're actually in pretty good shape right now. We

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-27 Thread Daniel Holbert
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 02/27/2014 10:26 AM, Zack Weinberg wrote: Does that mean a patch to squelch the uninitialized variable warnings in layout will now be accepted? Those are the only warnings in layout on my (Linux, debug) builds.

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-27 Thread Karl Tomlinson
Daniel Holbert writes: On 02/27/2014 10:26 AM, Zack Weinberg wrote: Does that mean a patch to squelch the uninitialized variable warnings in layout will now be accepted? Those are the only warnings in layout on my (Linux, debug) builds. layout/base/FrameLayerBuilder.cpp:3462:56

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-27 Thread Zack Weinberg
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 02/27/2014 03:11 PM, Karl Tomlinson wrote: Daniel Holbert writes: On 02/27/2014 10:26 AM, Zack Weinberg wrote: Does that mean a patch to squelch the uninitialized variable warnings in layout will now be accepted? Those are the only

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-27 Thread L. David Baron
On Thursday 2014-02-27 15:44 -0500, Zack Weinberg wrote: This is exactly the same thing dbaron said the last time I brought this up (quite some time ago - 2010, maybe?) I didn't buy it then and I don't buy it now. I think it is far more likely that a maybe-used-uninitialized true positive

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-27 Thread Botond Ballo
- Original Message - From: L. David Baron dba...@dbaron.org To: Zack Weinberg za...@panix.com Cc: dev-platform@lists.mozilla.org Sent: Thursday, February 27, 2014 3:57:56 PM Subject: Re: Fixing build warnings [was: Re: Always brace your ifs] On Thursday 2014-02-27 15:44 -0500, Zack

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-27 Thread Nicholas Nethercote
On Thu, Feb 27, 2014 at 12:44 PM, Zack Weinberg za...@panix.com wrote: Treating these as warnings, not errors, is probably the best thing here. If you see the warning and you've recently changed that code, then check it. If you haven't, you see the may be and ignore it. This is exactly

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-27 Thread Chris Peterson
On 2/27/14, 2:02 PM, Nicholas Nethercote wrote: So I'm pleased to hear that -W{sometimes,maybe}-initialized have lower false positive rates. Investigating them sounds like the most promising avenue for progress. Just to be clear: gcc's -Wmaybe-uninitialized is still very spammy. gcc's

Re: Always brace your ifs

2014-02-24 Thread Chris AtLee
On 17:37, Sat, 22 Feb, L. David Baron wrote: On Saturday 2014-02-22 15:57 -0800, Gregory Szorc wrote: On Feb 22, 2014, at 8:18, Kyle Huey m...@kylehuey.com wrote: If you needed another reason to follow the style guide: https://www.imperialviolet.org/2014/02/22/applebug.html Code coverage

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-24 Thread Sylvestre Ledru
On 24/02/2014 05:54, Daniel Holbert wrote: On 02/22/2014 12:26 PM, Hubert Figuière wrote: Now we talk about enabling more warning, yet Mozilla codebase is far from building warning free Maybe we should start with that first? FWIW, I (and others) have been working on that, as a side

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-24 Thread Daniel Holbert
On 02/24/2014 08:25 AM, Sylvestre Ledru wrote: By the way, do you have any plan to do the same with these libraries and forward the patches upstream? I don't have concrete plans to do this, but others are welcome to! It's often more difficult (with less immediate benefit) to fix warnings in

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-24 Thread Ralph Giles
On 2014-02-24 9:21 AM, Daniel Holbert wrote: a) We try to avoid directly modifying third-party code in m-c, since any such patches would be clobbered on the next library-update. So we may not be able to directly fix our in-tree copy of the code, unless it's really important. FWIW in the media

Re: Fixing build warnings [was: Re: Always brace your ifs]

2014-02-24 Thread Chris Peterson
On 2/23/14, 8:54 PM, Daniel Holbert wrote: We currently have only 100-200 build warnings[1], if you filter out warnings from third-party libraries that we import (e.g. cairo, skia, protobuf, ICU, various media codecs). On my machine, Firefox for OS X has 284 warnings, but only 24 are from

Re: Always brace your ifs

2014-02-23 Thread Gijs Kruitbosch
On 23/02/2014 00:33, Joshua Cranmer  wrote: On 2/22/2014 5:57 PM, Gregory Szorc wrote: On Feb 22, 2014, at 8:18, Kyle Huey m...@kylehuey.com wrote: If you needed another reason to follow the style guide: https://www.imperialviolet.org/2014/02/22/applebug.html Code coverage would have

Re: Always brace your ifs

2014-02-23 Thread Joshua Cranmer 
On 2/23/2014 4:06 PM, Gijs Kruitbosch wrote: Is there an accepted reporting format that'd be appropriate for output from such a tool? Perhaps the devtools folks could be interested in helping us out with something like this? I won't have time to look until after Australis has sailed. Code

Re: Always brace your ifs

2014-02-22 Thread Mike Hoye
On 2/22/2014, 1:04 PM, Jet Villegas wrote: goto ftw; I have to admit, I was very surprised to learn that: - Using both -Wall and -Wextra doesn't get you -Wunreachable-code. - The Clang manual lists documenting any of that that as a todo. - mhoye

Re: Always brace your ifs

2014-02-22 Thread Reuben Morais
On Feb 22, 2014, at 16:53, Mike Hoye mh...@mozilla.com wrote: On 2/22/2014, 1:04 PM, Jet Villegas wrote: goto ftw; I have to admit, I was very surprised to learn that: - Using both -Wall and -Wextra doesn't get you -Wunreachable-code. - The Clang manual lists documenting any of that that as

Re: Always brace your ifs

2014-02-22 Thread Hubert Figuière
On 22/02/14 02:53 PM, Mike Hoye wrote: On 2/22/2014, 1:04 PM, Jet Villegas wrote: goto ftw; I have to admit, I was very surprised to learn that: - Using both -Wall and -Wextra doesn't get you -Wunreachable-code. - The Clang manual lists documenting any of that that as a todo. Now we talk

Re: Always brace your ifs

2014-02-22 Thread Justin Dolske
On 2/22/14 7:18 AM, Kyle Huey wrote: If you needed another reason to follow the style guide: https://www.imperialviolet.org/2014/02/22/applebug.html I don't really disagree with bracing being a good idea, but I'll be contrarian here. Mandatory bracing probably wouldn't have helped; since you

Re: Always brace your ifs

2014-02-22 Thread Gregory Szorc
On Feb 22, 2014, at 8:18, Kyle Huey m...@kylehuey.com wrote: If you needed another reason to follow the style guide: https://www.imperialviolet.org/2014/02/22/applebug.html Code coverage would have caught this as well. The time investment into 100% line and branch coverage is debatable. But

Re: Always brace your ifs

2014-02-22 Thread Kyle Huey
On Sat, Feb 22, 2014 at 3:22 PM, Justin Dolske dol...@mozilla.com wrote: On 2/22/14 7:18 AM, Kyle Huey wrote: If you needed another reason to follow the style guide: https://www.imperialviolet.org/2014/02/22/applebug.html I don't really disagree with bracing being a good idea, but I'll be

Re: Always brace your ifs

2014-02-22 Thread Kyle Huey
On Sat, Feb 22, 2014 at 3:57 PM, Gregory Szorc g...@mozilla.com wrote: On Feb 22, 2014, at 8:18, Kyle Huey m...@kylehuey.com wrote: If you needed another reason to follow the style guide: https://www.imperialviolet.org/2014/02/22/applebug.html Code coverage would have caught this as well.

Re: Always brace your ifs

2014-02-22 Thread Joshua Cranmer 
On 2/22/2014 5:57 PM, Gregory Szorc wrote: On Feb 22, 2014, at 8:18, Kyle Huey m...@kylehuey.com wrote: If you needed another reason to follow the style guide: https://www.imperialviolet.org/2014/02/22/applebug.html Code coverage would have caught this as well. Actually, it probably

Re: Always brace your ifs

2014-02-22 Thread Joshua Cranmer 
On 2/22/2014 5:22 PM, Justin Dolske wrote: But really, the best way to fix this would be to use a macro: err = SSLHashSHA1.update(hashCtx, foo); SSL_ENSURE_SUCCESS(err, err); err = SSLHashSHA1.update(hashCtx, bar); SSL_ENSURE_SUCCESS(err, err); err = SSLHashSHA1.update(hashCtx, baz);

Re: Always brace your ifs

2014-02-22 Thread Kyle Huey
On Sat, Feb 22, 2014 at 4:38 PM, Joshua Cranmer  pidgeo...@gmail.com wrote: On 2/22/2014 5:22 PM, Justin Dolske wrote: But really, the best way to fix this would be to use a macro: err = SSLHashSHA1.update(hashCtx, foo); SSL_ENSURE_SUCCESS(err, err); err = SSLHashSHA1.update(hashCtx,

Re: Always brace your ifs

2014-02-22 Thread Neil
Joshua Cranmer wrote: Justin Dolske wrote: But really, the best way to fix this would be to use a macro: err = SSLHashSHA1.update(hashCtx, foo); SSL_ENSURE_SUCCESS(err, err); err = SSLHashSHA1.update(hashCtx, bar); SSL_ENSURE_SUCCESS(err, err); err = SSLHashSHA1.update(hashCtx,

Re: Always brace your ifs

2014-02-22 Thread L. David Baron
On Saturday 2014-02-22 15:57 -0800, Gregory Szorc wrote: On Feb 22, 2014, at 8:18, Kyle Huey m...@kylehuey.com wrote: If you needed another reason to follow the style guide: https://www.imperialviolet.org/2014/02/22/applebug.html Code coverage would have caught this as well. The time

Re: Always brace your ifs

2014-02-22 Thread Brian Smith
On Sat, Feb 22, 2014 at 5:06 PM, Neil n...@parkwaycc.co.uk wrote: Joshua Cranmer wrote: Being serious here, early-return and RTTI (to handle the cleanup prior to exit) would have eliminated the need for gotos in the first place. I assume you mean RAII. Unfortunately that requires C++. (I was