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

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

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

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

[Peter Kasting, 2014-03-18 03:02:51]

https://codereview.chromium.org/203013002/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/203013002/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:478: next_state_ =
STATE_WAITING_USER_ACTION;
(The code here was reordered to try and make it as structurally similar to the
surrounding cases as possible.)

https://codereview.chromium.org/203013002/diff/1/net/http/http_util.cc
File net/http/http_util.cc (left):

https://codereview.chromium.org/203013002/diff/1/net/http/http_util.cc#oldcode21
net/http/http_util.cc:21: using std::string;
(Much of the file already used std:: explicitly, so I just made the rest
consistent.)

https://codereview.chromium.org/203013002/diff/1/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/203013002/diff/1/net/http/http_util.cc#newcode35
net/http/http_util.cc:35: end != std::string::npos; end =
line.find_first_of(set, end + 2)) {
(This line relies on the fact that find_first_of() can take an offset beyond the
end of the string, and is guaranteed to return npos in that case.)

[Peter Kasting, 2014-03-19 21:10:46]

It seems akalin no longer works on Chromium.  Changing reviewer to rvargas.

[rvargas (doing something else), 2014-03-19 22:13:15]

https://codereview.chromium.org/203013002/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/203013002/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:510: }
nit: we should either have {} in all cases or we should remove them.

https://codereview.chromium.org/203013002/diff/1/net/spdy/spdy_network_transa...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/203013002/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:4233: FAIL() << "Unexpected read:
" << rv;
This just logs a message (and keeps track of the failure) as opposed to aborting
the test, right? Where is the old code unreachable?

I'm assuming that the change in logic (read only once vs read until pending) is
your way of saying if we're going to fail the test, why keep going (which sounds
right)

https://codereview.chromium.org/203013002/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:4236: data.CompleteRead();
This could end up being called when rv is not pending. Sounds better to avoid
that.

[Peter Kasting, 2014-03-19 22:17:09]

https://codereview.chromium.org/203013002/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/203013002/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:510: }
On 2014/03/19 22:13:15, rvargas wrote:
> nit: we should either have {} in all cases or we should remove them.

We can't remove the braces on this case, it declares a temporary inside the
block.  We don't need them in the cases that don't have them because they don't
declare any temps.  And the style guide doesn't suggest anywhere that you should
add extra braces when you don't need them.

Since this is stylistically correct (I would argue "better than any
alternative") and I'm not changing the presence of the braces on any of the
cases, I intend to leave this alone.

https://codereview.chromium.org/203013002/diff/1/net/spdy/spdy_network_transa...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/203013002/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:4233: FAIL() << "Unexpected read:
" << rv;
On 2014/03/19 22:13:15, rvargas wrote:
> This just logs a message (and keeps track of the failure) as opposed to
aborting
> the test, right?

Wrong.  FAIL() returns from the function.  You're thinking of ADD_FAILURE().

https://codereview.chromium.org/203013002/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:4236: data.CompleteRead();
On 2014/03/19 22:13:15, rvargas wrote:
> This could end up being called when rv is not pending.

No, it can't; see my reply above.

[rvargas (doing something else), 2014-03-19 22:56:24]

lgtm

https://codereview.chromium.org/203013002/diff/1/net/spdy/spdy_network_transa...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/203013002/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:4233: FAIL() << "Unexpected read:
" << rv;
On 2014/03/19 22:17:09, Peter Kasting wrote:
> On 2014/03/19 22:13:15, rvargas wrote:
> > This just logs a message (and keeps track of the failure) as opposed to
> aborting
> > the test, right?
> 
> Wrong.  FAIL() returns from the function.  You're thinking of ADD_FAILURE().

OK then. I still don't see the unreachable line in the old version.

At this point I would do
ASSERT_EQ(net::ERR_IO_PENDING, rv) << "foo"

[Peter Kasting, 2014-03-19 23:03:23]

https://codereview.chromium.org/203013002/diff/1/net/spdy/spdy_network_transa...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/203013002/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:4233: FAIL() << "Unexpected read:
" << rv;
On 2014/03/19 22:56:24, rvargas wrote:
> On 2014/03/19 22:17:09, Peter Kasting wrote:
> > On 2014/03/19 22:13:15, rvargas wrote:
> > > This just logs a message (and keeps track of the failure) as opposed to
> > aborting
> > > the test, right?
> > 
> > Wrong.  FAIL() returns from the function.  You're thinking of ADD_FAILURE().
> 
> OK then. I still don't see the unreachable line in the old version.

MSVC complains that the "while (rv > 0)" is an unreachable test.

> At this point I would do
> ASSERT_EQ(net::ERR_IO_PENDING, rv) << "foo"

Yeah, that is way better... dunno why I didn't think of that.  Done.

[Peter Kasting, 2014-03-19 23:04:03]

Committed patchset #2 manually as r258136 (presubmit successful).