Re: Always brace your ifs
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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 a todo. -Weverything ftw. -- reuben ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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 about enabling more warning, yet Mozilla codebase is far from building warning free Maybe we should start with that first? Hub ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: We live in a memory-constrained world
On Sat, Feb 22, 2014 at 11:22 PM, Andreas Gal andreas@gmail.com wrote: So, I'm wondering how much effort we should put in reducing the number of ChromeWorkers. We should continue to use JS in Chrome where it makes sense. Its often easier and faster to write some functionality in JS (and sometimes also safer), and it tends to be more compact when we ship it. In addition to purging caches and stopping idle workers the JS team is also working on making workers (and JS) more memory efficient in general. Naveed might want to chime in here. Brian Hackett's been making great progress in reducing the overhead of workers. https://bugzilla.mozilla.org/show_bug.cgi?id=964059 and https://bugzilla.mozilla.org/show_bug.cgi?id=964057 both landed this week, reducing the overhead of a trivial worker from ~1 MiB to ~0.2 MiB. And https://bugzilla.mozilla.org/show_bug.cgi?id=864927 is the meta-bug, which has ideas for further improvements. Nick ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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 could still easily end up with:
if ((err = SSLHashSHA1.update(hashCtx, foo)) != 0) {
goto error;
}
if ((err = SSLHashSHA1.update(hashCtx, bar)) != 0) {
goto error;
}
goto error;
if ((err = SSLHashSHA1.update(hashCtx, baz)) != 0) {
goto error;
}
There may even be an argument against bracing, in this specific case,
since it makes the repeated line slightly less visually obvious. I think
a remark about this was made in the recent style guide discussion: when
reviewing code, one often starts off by scanning for abnormal patterns
(hence of value of having consistent style, so abnormal isn't
conditional on which file you're in). But I don't really care. I think
the correctness-impact is usually exceedingly rare/minor, and the
ensuing style flamewars distract focus from better solutions.
For example, _both_ the Apple bug and my example above would have been
flagged by tools that warn about incorrect/misleading indentation.
Unsurprisingly, gps proposed doing this in the recent style guide thread
around automatic brace-insertion (and the risks thereto).
Dead/unreachable code analysis should have caught both as well, even
with correct indentation.
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);
SSL_ENSURE_SUCCESS(err, err);
Ok, maybe I'm not being entirely serious. :P
Justin
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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 you can't argue that code coverage has its place, especially for high-importance code such as crypto. AFAIK, our automation currently does not collect code coverage from any test suite. Should that change? Possibly radical idea: patches are automatically audited to verify tests actually test the new/changed code. If not, a giant flag is raised and reviewers can act accordingly. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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
contrarian here. Mandatory bracing probably wouldn't have helped; since you
could still easily end up with:
if ((err = SSLHashSHA1.update(hashCtx, foo)) != 0) {
goto error;
}
if ((err = SSLHashSHA1.update(hashCtx, bar)) != 0) {
goto error;
}
goto error;
if ((err = SSLHashSHA1.update(hashCtx, baz)) != 0) {
goto error;
}
There may even be an argument against bracing, in this specific case, since
it makes the repeated line slightly less visually obvious. I think a remark
about this was made in the recent style guide discussion: when reviewing
code, one often starts off by scanning for abnormal patterns (hence of value
of having consistent style, so abnormal isn't conditional on which file
you're in). But I don't really care. I think the correctness-impact is
usually exceedingly rare/minor, and the ensuing style flamewars distract
focus from better solutions.
For example, _both_ the Apple bug and my example above would have been
flagged by tools that warn about incorrect/misleading indentation.
Unsurprisingly, gps proposed doing this in the recent style guide thread
around automatic brace-insertion (and the risks thereto). Dead/unreachable
code analysis should have caught both as well, even with correct
indentation.
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);
SSL_ENSURE_SUCCESS(err, err);
Ok, maybe I'm not being entirely serious. :P
Justin
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Too bad everyone wants to get rid of our error code handling macros!
- Kyle
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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. The time investment into 100% line and branch coverage is debatable. But you can't argue that code coverage has its place, especially for high-importance code such as crypto. AFAIK, our automation currently does not collect code coverage from any test suite. Should that change? Possibly radical idea: patches are automatically audited to verify tests actually test the new/changed code. If not, a giant flag is raised and reviewers can act accordingly. This doesn't even need code coverage analysis, -Wunreachable-code would do. Code coverage is useful for seeing what code needs more tests written. I think we used the code coverage data that choller collected a year or two ago to pinpoint some IndexedDB features that needed better testing, although I can't find the relevant bugs now. It's certainly not a panacea though. For one thing, just executing a line of code doesn't mean a test will fail if that piece of code is broken. I would love to have that data available for identifying what needs more focused testing effort though. - Kyle ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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 wouldn't. The code after the goto which was unreachable would likely have been deleted from the CFG, certainly so in the optimized case. As deleted code, it wouldn't be emitted as being possibly executed, so you could still achieve 100% line coverage in those scenarios even being totally unable to reach lines of code. The time investment into 100% line and branch coverage is debatable. But you can't argue that code coverage has its place, especially for high-importance code such as crypto. AFAIK, our automation currently does not collect code coverage from any test suite. Should that change? I've tried getting code coverage working on our automation in the past. So far, it only works at all on Linux/Linux 32-bit. Windows is in a horrible situation to try to collect code coverage, and OS X and clang crop up other issues. I've not even attempted our Android or B2G builds. Also, 20-50% of our code is JS. There are extremely few tools that support JS code coverage, and none of them are capable of handling Mozilla's JS usage. -- Joshua Cranmer Thunderbird and DXR developer Source code archæologist ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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); SSL_ENSURE_SUCCESS(err, err); Ok, maybe I'm not being entirely serious. :P 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. -- Joshua Cranmer Thunderbird and DXR developer Source code archæologist ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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, bar); SSL_ENSURE_SUCCESS(err, err); err = SSLHashSHA1.update(hashCtx, baz); SSL_ENSURE_SUCCESS(err, err); Ok, maybe I'm not being entirely serious. :P 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. -- Joshua Cranmer Thunderbird and DXR developer Source code archæologist ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform Replacing goto with return is not sufficient. You're still vulnerable to the same mistake. - Kyle ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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, baz); SSL_ENSURE_SUCCESS(err, err); Ok, maybe I'm not being entirely serious. :P 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 fooled too; someone pointed out to me on IRC that update is actually a function pointer member. You reap what you sow.) -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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 investment into 100% line and branch coverage is debatable. But you can't argue that code coverage has its place, especially for high-importance code such as crypto. AFAIK, our automation currently does not collect code coverage from any test suite. Should that change? There was some automation running code coverage reports (with gcov, I think) for at least reftests + mochitests for an extended period of time; I found it useful for improving style system test coverage while it was running. I'm not sure how strong our commitment is to keeping such tests running, though; I frequently have to defend the tests against people who want to disable them because they take a long time (i.e., a long time for a single test file, which sometimes leads the tests to approach the per-file timeouts on slow VMs) or because they happen to exhibit the latest JIT crash frequently because they run a lot of code. I'm worried we're moving to a model where tests need to have active defenders to keep them running (even though that isn't how features on the Web platform work), because we blame the old test rather than the new regression. -David -- 턞 L. David Baron http://dbaron.org/ 턂 턢 Mozilla https://www.mozilla.org/ 턂 Before I built a wall I'd ask to know What I was walling in or walling out, And to whom I was like to give offense. - Robert Frost, Mending Wall (1914) signature.asc Description: Digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Always brace your ifs
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 fooled too; someone pointed out to me on IRC that update is actually a function pointer member. You reap what you sow.) I agree with Joshua. RAII requires C++ but requiring C++ is no big deal. RAII and early returns greatly help readability and thus security. This is exactly why the new certificate verification code is written in C++. Cheers, Brian -- Mozilla Networking/Crypto/Security (Necko/NSS/PSM) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
