Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2669263002:
Add missing case in MapFramerErrorToNetError (SpdyFramer::SPDY_OVERSIZED_PAYLOAD) (Closed)
|
Created:
3 years, 10 months ago by Maks Orlovich Modified:
3 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, bnc+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd missing case in MapFramerErrorToNetError (SpdyFramer::SPDY_OVERSIZED_PAYLOAD)
BUG=688004
Review-Url: https://codereview.chromium.org/2669263002
Cr-Commit-Position: refs/heads/master@{#448027}
Committed: https://chromium.googlesource.com/chromium/src/+/1531f72f4b6639759c9d81fb451cb662ebc54a11
Patch Set 1 #
Total comments: 8
Patch Set 2 : Apply feedback, to make the compiler catch it in the future. #
Total comments: 1
Patch Set 3 : Call NOTREACHED on invalid enum values, too... #Patch Set 4 : Merge with master changes. #Messages
Total messages: 38 (27 generated)
The CQ bit was checked by morlovich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
morlovich@chromium.org changed reviewers: + mmenke@chromium.org
Not certain the error code I chose is the right one.
mmenke@chromium.org changed reviewers: + bnc@chromium.org
Redirecting to bnc.
https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc#ne... net/spdy/spdy_session.cc:451: default: Suggest removing the default case from these - that will turn the NOTREACHED() into a compile time error instead, though I defer to Bence on this.
Description was changed from ========== Add missing case in MapFramerErrorToNetError (SpdyFramer::SPDY_OVERSIZED_PAYLOAD) BUG=688004 ========== to ========== Add missing case in MapFramerErrorToNetError (SpdyFramer::SPDY_OVERSIZED_PAYLOAD) BUG=688004 ==========
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
What Matt said. Thank you. https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc#ne... net/spdy/spdy_session.cc:411: return static_cast<SpdyProtocolErrorDetails>(-1); While you are at it, can you please also remove the default case here and move this return to after the switch(). https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc#ne... net/spdy/spdy_session.cc:451: default: On 2017/02/02 17:24:31, mmenke (Out Feb 4 to March 5) wrote: > Suggest removing the default case from these - that will turn the NOTREACHED() > into a compile time error instead, though I defer to Bence on this. Yes please. switch() without default case will make the compiler enforce that all values are handled. Of course you need to move the default case to after the switch(), because the function must return something for invalid values (e.g. when called with static_cast<SpdyFramerError>(1337)). https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc#ne... net/spdy/spdy_session.cc:490: return static_cast<SpdyProtocolErrorDetails>(-1); And also here please.
The CQ bit was checked by morlovich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc#ne... net/spdy/spdy_session.cc:411: return static_cast<SpdyProtocolErrorDetails>(-1); Done, though needed an extra case for LAST_ERROR. https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc#ne... net/spdy/spdy_session.cc:451: default: Likewise. https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc#ne... net/spdy/spdy_session.cc:490: return static_cast<SpdyProtocolErrorDetails>(-1); Likewise.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc#ne... net/spdy/spdy_session.cc:411: return static_cast<SpdyProtocolErrorDetails>(-1); On 2017/02/02 21:10:14, morlovich1 wrote: > Done, though needed an extra case for LAST_ERROR. Thanks. Oh, I hate that LAST_ERROR. https://codereview.chromium.org/2669263002/diff/20001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2669263002/diff/20001/net/spdy/spdy_session.c... net/spdy/spdy_session.cc:412: return static_cast<SpdyProtocolErrorDetails>(-1); Did you not want to add NOTREACHED() right above this line, to catch values greater than LAST_ERROR which are also invalid? Same for the other two functions below.
https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2669263002/diff/1/net/spdy/spdy_session.cc#ne... net/spdy/spdy_session.cc:411: return static_cast<SpdyProtocolErrorDetails>(-1); On 2017/02/02 21:10:14, morlovich1 wrote: > Done, though needed an extra case for LAST_ERROR. Thanks. Oh, I hate that LAST_ERROR. https://codereview.chromium.org/2669263002/diff/20001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/2669263002/diff/20001/net/spdy/spdy_session.c... net/spdy/spdy_session.cc:412: return static_cast<SpdyProtocolErrorDetails>(-1); Did you not want to add NOTREACHED() right above this line, to catch values greater than LAST_ERROR which are also invalid? Same for the other two functions below.
The CQ bit was checked by morlovich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by morlovich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by morlovich@chromium.org to run a CQ dry run
> https://codereview.chromium.org/2669263002/diff/20001/net/spdy/spdy_session.c... > net/spdy/spdy_session.cc:412: return static_cast<SpdyProtocolErrorDetails>(-1); > Did you not want to add NOTREACHED() right above this line, to catch values > greater than LAST_ERROR which are also invalid? Same for the other two > functions below. Done, and rebased. It's a bit less clear whether the NOTREACHED is a good idea in MapRstStreamStatusToProtocolError post-change, since the input enum now matches the wire one, and I am not sure if the wire value will get this far.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, thank you. On 2017/02/03 16:04:52, morlovich1 wrote: > > > https://codereview.chromium.org/2669263002/diff/20001/net/spdy/spdy_session.c... > > net/spdy/spdy_session.cc:412: return > static_cast<SpdyProtocolErrorDetails>(-1); > > Did you not want to add NOTREACHED() right above this line, to catch values > > greater than LAST_ERROR which are also invalid? Same for the other two > > functions below. > > Done, and rebased. It's a bit less clear whether the NOTREACHED is a good idea > in MapRstStreamStatusToProtocolError > post-change, since the input enum now matches the wire one, and I am not sure if > the wire value will get this far. The assumption is that MapRstStreamStatusToProtocolError() should only be called with a valid wire value. However, there must be a return value for any input value, even though we expect the code never to be called with certain values. I think NOTREACHED() is the best way to express this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by morlovich@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486142378146270,
"parent_rev": "6a67824709b57582fc3957f25b5abad81a90a324", "commit_rev":
"1531f72f4b6639759c9d81fb451cb662ebc54a11"}
Message was sent while issue was closed.
Description was changed from ========== Add missing case in MapFramerErrorToNetError (SpdyFramer::SPDY_OVERSIZED_PAYLOAD) BUG=688004 ========== to ========== Add missing case in MapFramerErrorToNetError (SpdyFramer::SPDY_OVERSIZED_PAYLOAD) BUG=688004 Review-Url: https://codereview.chromium.org/2669263002 Cr-Commit-Position: refs/heads/master@{#448027} Committed: https://chromium.googlesource.com/chromium/src/+/1531f72f4b6639759c9d81fb451c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1531f72f4b6639759c9d81fb451c... |
