Fix "unreachable code" warnings (MSVC warning 4702) in base/.

Fix "unreachable code" warnings (MSVC warning 4702) in base/.

This also does a little bit of code formatting in the hopes of shorter, more readable code.

BUG=346399
TEST=none
R=thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258398

[Peter Kasting, 2014-03-18 03:17:09]

https://codereview.chromium.org/202993003/diff/1/base/win/event_trace_consume...
File base/win/event_trace_consumer_unittest.cc (right):

https://codereview.chromium.org/202993003/diff/1/base/win/event_trace_consume...
base/win/event_trace_consumer_unittest.cc:58: if (event->MofData != NULL &&
event->MofLength != 0) {
(From our style guide: "Prefer (foo == 0) to (0 == foo).")

[Nico, 2014-03-18 10:29:47]

Please revert most of the style changes:
* "if (a) return b; return c;" vs "return a ? b : c;" is a subjective, the style
guide doesn't prefer either. So changes like this could be changed back and
forth over time, muddying up blame output without any benefit.
* Adding newlines in switches is also subjective and the style guide says "try
to minimize vertical space"
* etc

For the lines that you touch anyways for non-style reasons, reformatting is
fine.

The 0 == -> == 0 changes are fine, but please do these in a separate CL – right
now 90% of this 'Fix "unreachable code"' CL isn't related to fixing unreachable
code at all.

https://codereview.chromium.org/202993003/diff/1/base/test/trace_event_analyz...
File base/test/trace_event_analyzer.cc (right):

https://codereview.chromium.org/202993003/diff/1/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:835: return (FindEvents(query, &output) > 0) ?
output.front() : NULL;
no parens

https://codereview.chromium.org/202993003/diff/1/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:840: return (FindEvents(query, &output) > 0) ?
output.back() : NULL;
no parens

https://codereview.chromium.org/202993003/diff/1/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:924: if (query.Evaluate(*events[i])) {
at() does bounds checking, operator[] doesn't. Is this change safe?

[Peter Kasting, 2014-03-18 19:26:17]

I moved all style changes (that I thought weren't purely subjective) to
https://codereview.chromium.org/203223008/ .  I did my best to list exactly what
I was changing and why.  Feedback welcome there.

Consider this CL on hold until that lands.  Once that lands, this CL shouldn't
have any meaningful style changes and I'll re-ping on it.

https://codereview.chromium.org/202993003/diff/1/base/test/trace_event_analyz...
File base/test/trace_event_analyzer.cc (right):

https://codereview.chromium.org/202993003/diff/1/base/test/trace_event_analyz...
base/test/trace_event_analyzer.cc:924: if (query.Evaluate(*events[i])) {
On 2014/03/18 10:29:48, Nico (on GMT time Mar 15 - 24) wrote:
> at() does bounds checking, operator[] doesn't. Is this change safe?

at() bounds-checks by way of exceptions.  We don't support exceptions, so these
two aren't significantly different; they basically both crash on error.

[Nico, 2014-03-19 10:33:09]

On Tue, Mar 18, 2014 at 7:26 PM, <pkasting@chromium.org> wrote:

> I moved all style changes (that I thought weren't purely subjective) to
> https://codereview.chromium.org/203223008/ .  I did my best to list
> exactly what
> I was changing and why.  Feedback welcome there.
>
> Consider this CL on hold until that lands.  Once that lands, this CL
> shouldn't
> have any meaningful style changes and I'll re-ping on it.
>
>
>
> https://codereview.chromium.org/202993003/diff/1/base/
> test/trace_event_analyzer.cc
> File base/test/trace_event_analyzer.cc (right):
>
> https://codereview.chromium.org/202993003/diff/1/base/
> test/trace_event_analyzer.cc#newcode924
> base/test/trace_event_analyzer.cc:924: if (query.Evaluate(*events[i])) {
> On 2014/03/18 10:29:48, Nico (on GMT time Mar 15 - 24) wrote:
>
>> at() does bounds checking, operator[] doesn't. Is this change safe?
>>
>
> at() bounds-checks by way of exceptions.  We don't support exceptions,
> so these two aren't significantly different; they basically both crash
> on error.
>

operator[] isn't guaranteed to crash, it'll just read random memory and if
you're unlucky it'll read from another page that belongs to your program
and give you random data.


>
> https://codereview.chromium.org/202993003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2014-03-19 17:57:48]

On 2014/03/19 10:33:09, Nico (on GMT time Mar 15 - 24) wrote:
> > at() bounds-checks by way of exceptions.  We don't support exceptions,
> > so these two aren't significantly different; they basically both crash
> > on error.
> 
> operator[] isn't guaranteed to crash, it'll just read random memory and if
> you're unlucky it'll read from another page that belongs to your program
> and give you random data.

Yeah, I worded things badly.

This is effectively similar to replacing a CHECK with a DCHECK.  If you're
worried that you can actually have out-of-bounds array indexes, you need to
handle it separately so that those reads don't happen.  Otherwise, it's a bug
either way, regardless of whether the effect of the bug is to crash instantly or
not.  Because this is test code, making this change isn't going to cause
security problems for Chrome itself.  Given all of that, I think this change is
fine.

[Peter Kasting, 2014-03-19 21:34:50]

OK, PTAL.  The change now only has one minor style change in it, which is
something I overlooked when putting together my other patch.

https://codereview.chromium.org/202993003/diff/20001/base/win/event_trace_con...
File base/win/event_trace_consumer_unittest.cc (right):

https://codereview.chromium.org/202993003/diff/20001/base/win/event_trace_con...
base/win/event_trace_consumer_unittest.cc:236: test_provider_,
TRACE_LEVEL_VERBOSE, 0xFFFFFFFF));
I forgot to fix this in my style change when I fixed line 291-292.

[Nico, 2014-03-20 16:08:49]

On Wed, Mar 19, 2014 at 5:57 PM, <pkasting@chromium.org> wrote:

> On 2014/03/19 10:33:09, Nico (on GMT time Mar 15 - 24) wrote:
>
>> > at() bounds-checks by way of exceptions.  We don't support exceptions,
>> > so these two aren't significantly different; they basically both crash
>> > on error.
>>
>
>  operator[] isn't guaranteed to crash, it'll just read random memory and if
>> you're unlucky it'll read from another page that belongs to your program
>> and give you random data.
>>
>
> Yeah, I worded things badly.
>
> This is effectively similar to replacing a CHECK with a DCHECK.  If you're
> worried that you can actually have out-of-bounds array indexes, you need to
> handle it separately so that those reads don't happen.  Otherwise, it's a
> bug
> either way, regardless of whether the effect of the bug is to crash
> instantly or
> not.


Crashing is much better than invalid reads, because the latter class of
bugs is exploitable and the former isn't. (I checked for the CHECK ->
DCHECK conversions you did that release builds to a clean nullpointer deref
with the DCHECK compiled out, for this reason.)


> Because this is test code, making this change isn't going to cause
> security problems for Chrome itself.  Given all of that, I think this
> change is
> fine.


This part of the argument is convincing enough though.


>
> https://codereview.chromium.org/202993003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-03-20 16:12:24]

lgtm

Thanks for splitting this up, this made it much easier to understand what's
going on.

(Looks like the at() change is gone.)

[Peter Kasting, 2014-03-20 20:37:46]

Committed patchset #3 manually as r258398 (presubmit successful).