Fix "value possibly truncated" warnings on MSVC, net/spdy/ edition.

Fix "value possibly truncated" warnings on MSVC, net/spdy/ edition.

Specifically, this addresses some instances of C4244 ( http://msdn.microsoft.com/en-us/library/th7a07tz.aspx ).

Also does a few misc. cleanups, mostly to test code.

Note: This CL includes server-side change 81444427.

BUG=81439
TEST=none

Committed: https://crrev.com/1662810517ea5b08e4e334b3eb9450833c148fb6
Cr-Commit-Position: refs/heads/master@{#307099}

[Peter Kasting, 2014-11-25 22:50:59]

pkasting@chromium.org changed reviewers:
+ rch@chromium.org

[Peter Kasting, 2014-11-25 22:51:01]

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
File net/spdy/hpack_huffman_table_test.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
net/spdy/hpack_huffman_table_test.cc:241: EXPECT_EQ(arraysize(code),
peer_.code_by_id().size());
This is a more compact and future-proof way of writing this (if in the future
someone changes the values above, they won't need to change them in two places).

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (left):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc#...
net/spdy/spdy_framer.cc:739: SpdyConstants::DataFrameType(protocol_version());
This initialization is redundant with the one above the conditional.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc#...
net/spdy/spdy_framer.cc:746: is_control_frame = control_frame_type_field !=
This is just a shorter version of the original statement.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc#...
net/spdy/spdy_framer.cc:2541: DCHECK_GE(setting_id, 0);
SerializeSettingId() can return -1 on error, but the code below assumes there's
no error.  I assumed the code was correct and added this DCHECK as documentation
of that assumption.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc#...
net/spdy/spdy_framer.cc:2860:
builder.WriteUInt8(static_cast<uint8>(altsvc.protocol_id().length()));
I assumed the lengths here and below were guaranteed to fit in a uint8 and there
was no need to check or DCHECK them.  Let me know if this isn't true.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer_tes...
File net/spdy/spdy_framer_test.cc (left):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer_tes...
net/spdy/spdy_framer_test.cc:27: using testing::Pair;
Most of the changes in this file are from removing these.  (In general, we try
to avoid using directives unless using them significantly improves readability.)

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer_tes...
File net/spdy/spdy_framer_test.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer_tes...
net/spdy/spdy_framer_test.cc:3791: std::string pad(pad_length, 'A');
This was an outright error!  It worries me that the test didn't seem to mind.

[Ryan Hamilton, 2014-11-27 17:00:05]

rch@chromium.org changed reviewers:
+ bnc@chromium.org

[Ryan Hamilton, 2014-11-27 17:00:06]

I'm going to hand this off to bnc@ since it includes changes to code that will
need to be merged back to the internal repository...

[Bence, 2014-12-01 18:18:38]

Peter: nits.  I did a preliminary upstreaming to internal code, and tests pass
there too.

I couldn't find the error code for "value possibly truncated".  Could you please
update the CL description with a link like
http://msdn.microsoft.com/en-us/library/7hw7c1he.aspx, but corresponding to this
error?  Thanks.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
File net/spdy/hpack_huffman_table.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
net/spdy/hpack_huffman_table.cc:60: return false;
Could this be a DCHECK to document requirements?  We are not expecting the
caller do to error handling anyway.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
File net/spdy/hpack_huffman_table_test.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
net/spdy/hpack_huffman_table_test.cc:241: EXPECT_EQ(arraysize(code),
peer_.code_by_id().size());
On 2014/11/25 22:51:00, Peter Kasting wrote:
> This is a more compact and future-proof way of writing this (if in the future
> someone changes the values above, they won't need to change them in two
places).

Are you sure it is a good idea, considering TotT Episode 338 "Don't Put Logic in
Tests"?  I think the original is more direct and reads more easily.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc#...
net/spdy/spdy_framer.cc:2860:
builder.WriteUInt8(static_cast<uint8>(altsvc.protocol_id().length()));
On 2014/11/25 22:51:00, Peter Kasting wrote:
> I assumed the lengths here and below were guaranteed to fit in a uint8 and
there
> was no need to check or DCHECK them.  Let me know if this isn't true.

SpdyAltSvcIR.host_ and protocol_id_ are std::strings, and set_host() and
set_protocol_id() do not enforce a limit on length.  Could you please add
DCHECKs on length here (or possibly to the set_* methods in spdy_protocol.h)?

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer_tes...
File net/spdy/spdy_framer_test.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer_tes...
net/spdy/spdy_framer_test.cc:3770: 0x00, 0x00, 0x00, static_cast<unsigned
char>(length + 1),
Why do you add 1 here?

[Peter Kasting, 2014-12-02 02:13:56]

PTAL.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
File net/spdy/hpack_huffman_table.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
net/spdy/hpack_huffman_table.cc:60: return false;
On 2014/12/01 18:18:38, Bence wrote:
> Could this be a DCHECK to document requirements?  We are not expecting the
> caller do to error handling anyway.

Changed to a DCHECK and added a note to the declaration comment.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
File net/spdy/hpack_huffman_table_test.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
net/spdy/hpack_huffman_table_test.cc:241: EXPECT_EQ(arraysize(code),
peer_.code_by_id().size());
On 2014/12/01 18:18:38, Bence wrote:
> On 2014/11/25 22:51:00, Peter Kasting wrote:
> > This is a more compact and future-proof way of writing this (if in the
future
> > someone changes the values above, they won't need to change them in two
> places).
> 
> Are you sure it is a good idea, considering TotT Episode 338 "Don't Put Logic
in
> Tests"?  I think the original is more direct and reads more easily.

Yes, I think the change is good even taking that episode into account.  That
episode discusses performing computations in tests to construct inputs and
outputs.  This isn't "computing" anything about the inputs and outputs, it's
simply saying that the initialized table should have the exact value we passed
in.

The previous code had a list of numbers for the lengths whose relationship with
the initializers above wasn't instantly clear.  A maintainer trying to edit the
list above wouldn't necessarily see this or know it should be changed, and if so
how.

The whole change here was originally prompted by some type conversion warnings
the second EXPECT_THAT generated, although I don't recall at this point
precisely what the warnings were.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc#...
net/spdy/spdy_framer.cc:2860:
builder.WriteUInt8(static_cast<uint8>(altsvc.protocol_id().length()));
On 2014/12/01 18:18:38, Bence wrote:
> On 2014/11/25 22:51:00, Peter Kasting wrote:
> > I assumed the lengths here and below were guaranteed to fit in a uint8 and
> there
> > was no need to check or DCHECK them.  Let me know if this isn't true.
> 
> SpdyAltSvcIR.host_ and protocol_id_ are std::strings, and set_host() and
> set_protocol_id() do not enforce a limit on length.  Could you please add
> DCHECKs on length here (or possibly to the set_* methods in spdy_protocol.h)?

Added DCHECKs to the setters, but perhaps these should be CHECKs instead? 
Otherwise it seems like we get a corrupted serialization leading to who knows
what.

That said, AFAICT, those two setters are only ever used by test code.  So
outside of tests, these fields are always empty?  Maybe they should disappear or
something.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer_tes...
File net/spdy/spdy_framer_test.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer_tes...
net/spdy/spdy_framer_test.cc:3770: 0x00, 0x00, 0x00, static_cast<unsigned
char>(length + 1),
On 2014/12/01 18:18:38, Bence wrote:
> Why do you add 1 here?

Bug from a modified previous version of rewriting this :(

Apparently the test didn't fail with this bug either, which may also be
worrisome...

[Bence, 2014-12-02 14:58:44]

Peter: just one more thing please.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
File net/spdy/hpack_huffman_table_test.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/hpack_huffman_t...
net/spdy/hpack_huffman_table_test.cc:241: EXPECT_EQ(arraysize(code),
peer_.code_by_id().size());
On 2014/12/02 02:13:55, Peter Kasting wrote:
> On 2014/12/01 18:18:38, Bence wrote:
> > On 2014/11/25 22:51:00, Peter Kasting wrote:
> > > This is a more compact and future-proof way of writing this (if in the
> future
> > > someone changes the values above, they won't need to change them in two
> > places).
> > 
> > Are you sure it is a good idea, considering TotT Episode 338 "Don't Put
Logic
> in
> > Tests"?  I think the original is more direct and reads more easily.
> 
> Yes, I think the change is good even taking that episode into account.  That
> episode discusses performing computations in tests to construct inputs and
> outputs.  This isn't "computing" anything about the inputs and outputs, it's
> simply saying that the initialized table should have the exact value we passed
> in.
> 
> The previous code had a list of numbers for the lengths whose relationship
with
> the initializers above wasn't instantly clear.  A maintainer trying to edit
the
> list above wouldn't necessarily see this or know it should be changed, and if
so
> how.
> 
> The whole change here was originally prompted by some type conversion warnings
> the second EXPECT_THAT generated, although I don't recall at this point
> precisely what the warnings were.

Okay, you convinced me.

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc#...
net/spdy/spdy_framer.cc:2860:
builder.WriteUInt8(static_cast<uint8>(altsvc.protocol_id().length()));
On 2014/12/02 02:13:55, Peter Kasting wrote:
> Added DCHECKs to the setters, but perhaps these should be CHECKs instead? 
> Otherwise it seems like we get a corrupted serialization leading to who knows
> what.

Hey, I just realized this binary format is obsolete.  Sorry.  This format was
specified in
https://tools.ietf.org/html/draft-ietf-httpbis-alt-svc-04#section-4, but has
been changed in
https://tools.ietf.org/html/draft-ietf-httpbis-alt-svc-05#section-4, and does
not have the length<256 limitation any more.

Could you please delete the DCHECKs from the setters, and add a TODO(bnc) here.

> That said, AFAICT, those two setters are only ever used by test code.  So
> outside of tests, these fields are always empty?  Maybe they should disappear
or
> something.

Indeed, and SerializeAltSvc is not currently used in Chrome, nor in the internal
codebase.  We still need the setters for the future when we start emitting
AltSvc frames.  (Or initializing in the constructor, or something.)

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer_tes...
File net/spdy/spdy_framer_test.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer_tes...
net/spdy/spdy_framer_test.cc:3770: 0x00, 0x00, 0x00, static_cast<unsigned
char>(length + 1),
On 2014/12/02 02:13:55, Peter Kasting wrote:
> On 2014/12/01 18:18:38, Bence wrote:
> > Why do you add 1 here?
> 
> Bug from a modified previous version of rewriting this :(
> 
> Apparently the test didn't fail with this bug either, which may also be
> worrisome...

FYI I tried many different values here locally and the test never failed...

[Bence, 2014-12-02 17:28:00]

Please link to https://crbug.com/438263 in the TODO(bnc) comment if you have not
had a chance to start working on this yet.  Thanks.

> Hey, I just realized this binary format is obsolete.  Sorry.  This format was
> specified in
> https://tools.ietf.org/html/draft-ietf-httpbis-alt-svc-04#section-4, but has
> been changed in
> https://tools.ietf.org/html/draft-ietf-httpbis-alt-svc-05#section-4, and does
> not have the length<256 limitation any more.
> 
> Could you please delete the DCHECKs from the setters, and add a TODO(bnc)
here.
>

[Peter Kasting, 2014-12-03 00:27:08]

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

https://codereview.chromium.org/759063003/diff/20001/net/spdy/spdy_framer.cc#...
net/spdy/spdy_framer.cc:2860:
builder.WriteUInt8(static_cast<uint8>(altsvc.protocol_id().length()));
On 2014/12/02 14:58:44, Bence wrote:
> On 2014/12/02 02:13:55, Peter Kasting wrote:
> > Added DCHECKs to the setters, but perhaps these should be CHECKs instead? 
> > Otherwise it seems like we get a corrupted serialization leading to who
knows
> > what.
> 
> Hey, I just realized this binary format is obsolete.  Sorry.  This format was
> specified in
> https://tools.ietf.org/html/draft-ietf-httpbis-alt-svc-04#section-4, but has
> been changed in
> https://tools.ietf.org/html/draft-ietf-httpbis-alt-svc-05#section-4, and does
> not have the length<256 limitation any more.
> 
> Could you please delete the DCHECKs from the setters, and add a TODO(bnc)
here.
> 
> > That said, AFAICT, those two setters are only ever used by test code.  So
> > outside of tests, these fields are always empty?  Maybe they should
disappear
> or
> > something.
> 
> Indeed, and SerializeAltSvc is not currently used in Chrome, nor in the
internal
> codebase.  We still need the setters for the future when we start emitting
> AltSvc frames.  (Or initializing in the constructor, or something.)

Done.

[Bence, 2014-12-03 22:48:24]

Peter: this looks fine to me, but please give me some time to work on the
internal codebase so that this change can be landed.

[Bence, 2014-12-03 23:23:21]

Peter: unfortunately there is one issue that needs remedy.

https://codereview.chromium.org/759063003/diff/120001/net/spdy/spdy_framer_te...
File net/spdy/spdy_framer_test.cc (left):

https://codereview.chromium.org/759063003/diff/120001/net/spdy/spdy_framer_te...
net/spdy/spdy_framer_test.cc:22: using std::string;
Could you please undo the removal of only this one line, and
s/std::string/sting/ in this file?  There is a string-specific namespace issue
in the internal codebase which prevents this change from being merged, and
divergence between Chromium and that codebase is undesirable for maintenance
reasons.

[Peter Kasting, 2014-12-03 23:33:13]

https://codereview.chromium.org/759063003/diff/120001/net/spdy/spdy_framer_te...
File net/spdy/spdy_framer_test.cc (left):

https://codereview.chromium.org/759063003/diff/120001/net/spdy/spdy_framer_te...
net/spdy/spdy_framer_test.cc:22: using std::string;
On 2014/12/03 23:23:20, Bence wrote:
> Could you please undo the removal of only this one line, and
> s/std::string/sting/ in this file?  There is a string-specific namespace issue
> in the internal codebase which prevents this change from being merged, and
> divergence between Chromium and that codebase is undesirable for maintenance
> reasons.

Sure.  What's the issue?  Maybe I can help come up with a solution.

[Peter Kasting, 2014-12-05 03:48:44]

Reverted "using std::string" deletion.  I also changed one other member in this
file that was using "std::string" already to use "string".

[Bence, 2014-12-05 17:22:01]

Peter: one more nit.  Thanks.

https://codereview.chromium.org/759063003/diff/140001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

https://codereview.chromium.org/759063003/diff/140001/net/spdy/spdy_framer.cc...
net/spdy/spdy_framer.cc:747: ((protocol_version() > SPDY3) ?
From line 697, we know that protocol() > SPDY3.  This allows us to simplify this
statement.

[Peter Kasting, 2014-12-05 19:21:28]

If you have only nits and are OK with a change landing after they're addressed,
saying LGTM along with your nits saves an extra review cycle.

https://codereview.chromium.org/759063003/diff/140001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

https://codereview.chromium.org/759063003/diff/140001/net/spdy/spdy_framer.cc...
net/spdy/spdy_framer.cc:747: ((protocol_version() > SPDY3) ?
On 2014/12/05 17:22:01, Bence wrote:
> From line 697, we know that protocol() > SPDY3.  This allows us to simplify
this
> statement.

Done.

[Bence, 2014-12-05 21:00:02]

On 2014/12/05 19:21:28, Peter Kasting wrote:
> If you have only nits and are OK with a change landing after they're
addressed,
> saying LGTM along with your nits saves an extra review cycle.
> 

Peter:  Thank you.  Please add "This CL includes server side change 81444427."
to the commit message.  LGTM.

Note that I am not an owner but a committer.  You might need rch's rubber stamp.

[Ryan Hamilton, 2014-12-05 21:47:01]

sorry for the delay.

lgtm

[Peter Kasting, 2014-12-05 21:49:24]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-12-05 21:50:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759063003/160001

[commit-bot: I haz the power, 2014-12-05 22:49:51]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-12-05 22:50:42]

Patchset 9 (id:??) landed as
https://crrev.com/1662810517ea5b08e4e334b3eb9450833c148fb6
Cr-Commit-Position: refs/heads/master@{#307099}