Fixes for re-enabling more MSVC level 4 warnings: net/quic/ edition

Fixes for re-enabling more MSVC level 4 warnings: net/quic/ edition

This contains fixes for the following sorts of issues:
* Signedness mismatch
* Possibly-uninitialized local variable

This also contains one cleanup change (to remove an extra set of parens).

BUG=81439
TEST=none
R=rch@chromium.org

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

[Peter Kasting, 2014-07-08 02:16:48]

Please note my concerns in quic_reliable_client_stream.cc, where I think
returning an error code from ProcessData() can lead callers to do Bad Things
(based on briefly auditing the callsites).

[Ryan Hamilton, 2014-07-08 03:37:08]

LGTM. I'll take care of merging these changes to the internal repository.

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
File net/quic/crypto/strike_register.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
net/quic/crypto/strike_register.cc:18: const uint32 StrikeRegister::kNil =
0x80000001;
Out of curiosity, why this change?

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
File net/quic/quic_reliable_client_stream.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
net/quic/quic_reliable_client_stream.cc:33: return
static_cast<uint32>(ERR_ABORTED);
Yes, this seems problematic. :> It should probably do something very similar to
the rv != OK block below. I'll take care of fixing this in a different CL.

[Peter Kasting, 2014-07-08 06:26:46]

> LGTM. I'll take care of merging these changes to the internal repository.

I didn't know there was an internal repository.  Presumably that's completely
separated from the process of getting this change landed and I should proceed
with this change as if you never said anything :)

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
File net/quic/crypto/strike_register.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
net/quic/crypto/strike_register.cc:18: const uint32 StrikeRegister::kNil =
0x80000001;
On 2014/07/08 03:37:08, Ryan Hamilton wrote:
> Out of curiosity, why this change?

1 << 31 is a (negative) signed value (because 1 is a signed value), thus
triggering the "signedness mismatch" warning.

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
File net/quic/quic_reliable_client_stream.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
net/quic/quic_reliable_client_stream.cc:33: return
static_cast<uint32>(ERR_ABORTED);
On 2014/07/08 03:37:08, Ryan Hamilton wrote:
> Yes, this seems problematic. :> It should probably do something very similar
to
> the rv != OK block below. I'll take care of fixing this in a different CL.

So do you want me to just remove the comment and leave the static cast here for
now?

[Ryan Hamilton, 2014-07-08 18:00:57]

lgtm

Thanks!

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
File net/quic/crypto/strike_register.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
net/quic/crypto/strike_register.cc:18: const uint32 StrikeRegister::kNil =
0x80000001;
On 2014/07/08 06:26:46, Peter Kasting wrote:
> On 2014/07/08 03:37:08, Ryan Hamilton wrote:
> > Out of curiosity, why this change?
> 
> 1 << 31 is a (negative) signed value (because 1 is a signed value), thus
> triggering the "signedness mismatch" warning.

AH. In that case, how about 1u << 31?  (We end up having to use 1u all the time
in EXPECT_EQ() for this reason).

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
File net/quic/quic_reliable_client_stream.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
net/quic/quic_reliable_client_stream.cc:33: return
static_cast<uint32>(ERR_ABORTED);
On 2014/07/08 06:26:46, Peter Kasting wrote:
> On 2014/07/08 03:37:08, Ryan Hamilton wrote:
> > Yes, this seems problematic. :> It should probably do something very similar
> to
> > the rv != OK block below. I'll take care of fixing this in a different CL.
> 
> So do you want me to just remove the comment and leave the static cast here
for
> now?

Either way. The comment is a good reminder in any case :>

[Peter Kasting, 2014-07-08 18:16:47]

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
File net/quic/crypto/strike_register.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
net/quic/crypto/strike_register.cc:18: const uint32 StrikeRegister::kNil =
0x80000001;
On 2014/07/08 18:00:57, Ryan Hamilton wrote:
> On 2014/07/08 06:26:46, Peter Kasting wrote:
> > On 2014/07/08 03:37:08, Ryan Hamilton wrote:
> > > Out of curiosity, why this change?
> > 
> > 1 << 31 is a (negative) signed value (because 1 is a signed value), thus
> > triggering the "signedness mismatch" warning.
> 
> AH. In that case, how about 1u << 31?  (We end up having to use 1u all the
time
> in EXPECT_EQ() for this reason).

That would certainly compile without error.  I personally find the hex form a
little nicer because to me it emphasizes the 32-bitness of the variable, but I'm
not wedded to it.  I can do whatever you prefer.  Should I change it?

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
File net/quic/quic_reliable_client_stream.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
net/quic/quic_reliable_client_stream.cc:33: return
static_cast<uint32>(ERR_ABORTED);
On 2014/07/08 18:00:57, Ryan Hamilton wrote:
> On 2014/07/08 06:26:46, Peter Kasting wrote:
> > On 2014/07/08 03:37:08, Ryan Hamilton wrote:
> > > Yes, this seems problematic. :> It should probably do something very
similar
> > to
> > > the rv != OK block below. I'll take care of fixing this in a different CL.
> > 
> > So do you want me to just remove the comment and leave the static cast here
> for
> > now?
> 
> Either way. The comment is a good reminder in any case :>

I think what I'll do, then, is change to something like this, assuming you don't
mind:

  // TODO(rch): buffer data if we don't have a delegate.
  // Also, if we're going to return 0 here, we should probably do something
  // like the "rv != OK" code below.
  if (!delegate_)
    return 0;

[Ryan Hamilton, 2014-07-08 18:24:01]

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
File net/quic/crypto/strike_register.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
net/quic/crypto/strike_register.cc:18: const uint32 StrikeRegister::kNil =
0x80000001;
On 2014/07/08 18:16:46, Peter Kasting wrote:
> On 2014/07/08 18:00:57, Ryan Hamilton wrote:
> > On 2014/07/08 06:26:46, Peter Kasting wrote:
> > > On 2014/07/08 03:37:08, Ryan Hamilton wrote:
> > > > Out of curiosity, why this change?
> > > 
> > > 1 << 31 is a (negative) signed value (because 1 is a signed value), thus
> > > triggering the "signedness mismatch" warning.
> > 
> > AH. In that case, how about 1u << 31?  (We end up having to use 1u all the
> time
> > in EXPECT_EQ() for this reason).
> 
> That would certainly compile without error.  I personally find the hex form a
> little nicer because to me it emphasizes the 32-bitness of the variable, but
I'm
> not wedded to it.  I can do whatever you prefer.  Should I change it?

Personally, I'd prefer 1u << 31 because when I see 0x8....1, I can't tell how
big that value is. They're both totally reasonable, of course.

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
File net/quic/quic_reliable_client_stream.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
net/quic/quic_reliable_client_stream.cc:33: return
static_cast<uint32>(ERR_ABORTED);
On 2014/07/08 18:16:46, Peter Kasting wrote:
> On 2014/07/08 18:00:57, Ryan Hamilton wrote:
> > On 2014/07/08 06:26:46, Peter Kasting wrote:
> > > On 2014/07/08 03:37:08, Ryan Hamilton wrote:
> > > > Yes, this seems problematic. :> It should probably do something very
> similar
> > > to
> > > > the rv != OK block below. I'll take care of fixing this in a different
CL.
> > > 
> > > So do you want me to just remove the comment and leave the static cast
here
> > for
> > > now?
> > 
> > Either way. The comment is a good reminder in any case :>
> 
> I think what I'll do, then, is change to something like this, assuming you
don't
> mind:
> 
>   // TODO(rch): buffer data if we don't have a delegate.
>   // Also, if we're going to return 0 here, we should probably do something
>   // like the "rv != OK" code below.
>   if (!delegate_)
>     return 0;

Sure, that's fine. Or you can simply do:

  if (!delegate_) {
    DLOG(ERROR) << "Missing delegate";
    Reset(QUIC_STREAM_CANCELLED);
    return 0;
  }

Which I was planning to land in https://codereview.chromium.org/372343002/ But
if you want to do it here, that's totally good too.

[Peter Kasting, 2014-07-08 18:44:45]

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
File net/quic/crypto/strike_register.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
net/quic/crypto/strike_register.cc:18: const uint32 StrikeRegister::kNil =
0x80000001;
On 2014/07/08 18:24:01, Ryan Hamilton wrote:
> Personally, I'd prefer 1u << 31 because when I see 0x8....1, I can't tell how
> big that value is.

Changed.

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
File net/quic/quic_reliable_client_stream.cc (right):

https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
net/quic/quic_reliable_client_stream.cc:33: return
static_cast<uint32>(ERR_ABORTED);
On 2014/07/08 18:24:01, Ryan Hamilton wrote:
> Sure, that's fine. Or you can simply do:
> 
>   if (!delegate_) {
>     DLOG(ERROR) << "Missing delegate";
>     Reset(QUIC_STREAM_CANCELLED);
>     return 0;
>   }
> 
> Which I was planning to land in https://codereview.chromium.org/372343002/ But
> if you want to do it here, that's totally good too.

Ah!  I'd rather let you make the functional change through the normal review
process if you've already got it in the pipeline.  I'll just revert my change to
this file and leave it in your hands.

[Peter Kasting, 2014-07-08 18:44:50]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-08 18:48:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkasting@chromium.org/371213002/30001

[Ryan Hamilton, 2014-07-08 18:56:13]

On 2014/07/08 18:44:45, Peter Kasting wrote:
>
https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
> File net/quic/crypto/strike_register.cc (right):
> 
>
https://codereview.chromium.org/371213002/diff/10007/net/quic/crypto/strike_r...
> net/quic/crypto/strike_register.cc:18: const uint32 StrikeRegister::kNil =
> 0x80000001;
> On 2014/07/08 18:24:01, Ryan Hamilton wrote:
> > Personally, I'd prefer 1u << 31 because when I see 0x8....1, I can't tell
how
> > big that value is.
> 
> Changed.
> 
>
https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
> File net/quic/quic_reliable_client_stream.cc (right):
> 
>
https://codereview.chromium.org/371213002/diff/10007/net/quic/quic_reliable_c...
> net/quic/quic_reliable_client_stream.cc:33: return
> static_cast<uint32>(ERR_ABORTED);
> On 2014/07/08 18:24:01, Ryan Hamilton wrote:
> > Sure, that's fine. Or you can simply do:
> > 
> >   if (!delegate_) {
> >     DLOG(ERROR) << "Missing delegate";
> >     Reset(QUIC_STREAM_CANCELLED);
> >     return 0;
> >   }
> > 
> > Which I was planning to land in https://codereview.chromium.org/372343002/
But
> > if you want to do it here, that's totally good too.
> 
> Ah!  I'd rather let you make the functional change through the normal review
> process if you've already got it in the pipeline.  I'll just revert my change
to
> this file and leave it in your hands.

Thanks for doing all this!

[commit-bot: I haz the power, 2014-07-08 22:26:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-08 22:31:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-08 22:31:11]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[Peter Kasting, 2014-07-08 23:17:34]

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