Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2448193007:
Fix a redundant expression (Closed)
DescriptionFix a redundant expression
Found by static analysis with PVS-Studio!
BUG=660198
Committed: https://crrev.com/8f240d5ca7cad065a2dd72fb819537914319c806
Cr-Commit-Position: refs/heads/master@{#428882}
Patch Set 1 #
Total comments: 3
Patch Set 2 : check request_body_read_buf_ == nullptr #Messages
Total messages: 15 (6 generated)
hans@chromium.org changed reviewers: + maksim.sisov@intel.com, mattm@chromium.org
Please take a look.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2448193007/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (left): https://codereview.chromium.org/2448193007/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:1234: request_body_send_buf_ == nullptr; Based on line 610, are you sure this wasn't intended to be a check of |request_body_read_buf_|?
https://codereview.chromium.org/2448193007/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (left): https://codereview.chromium.org/2448193007/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:1234: request_body_send_buf_ == nullptr; On 2016/10/27 22:31:36, Peter Kasting wrote: > Based on line 610, are you sure this wasn't intended to be a check of > |request_body_read_buf_|? The name suggests it's intending to check if *send* buffers are empty, but your pointer to line 610 does make me unsure. maksims might be able to give a more definite answer.
https://codereview.chromium.org/2448193007/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (left): https://codereview.chromium.org/2448193007/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:1234: request_body_send_buf_ == nullptr; On 2016/10/27 22:39:09, hans wrote: > On 2016/10/27 22:31:36, Peter Kasting wrote: > > Based on line 610, are you sure this wasn't intended to be a check of > > |request_body_read_buf_|? > > The name suggests it's intending to check if *send* buffers are empty, but your > pointer to line 610 does make me unsure. > > maksims might be able to give a more definite answer. Oh, that was a typo. It should have been a *read* buffer. Firstly, data is read from UploadDataStream into *read* buf and then encoded into *send* buff. When the request is sent, buffers are flushed.
Description was changed from ========== Remove a redundant expression Found by static analysis with PVS-Studio! BUG=660198 ========== to ========== Fix a redundant expression Found by static analysis with PVS-Studio! BUG=660198 ==========
Thanks for clarifying. Please take another look.
On 2016/10/28 16:37:09, hans wrote: > Thanks for clarifying. Please take another look. lgtm based on comment #6.
The CQ bit was checked by hans@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix a redundant expression Found by static analysis with PVS-Studio! BUG=660198 ========== to ========== Fix a redundant expression Found by static analysis with PVS-Studio! BUG=660198 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix a redundant expression Found by static analysis with PVS-Studio! BUG=660198 ========== to ========== Fix a redundant expression Found by static analysis with PVS-Studio! BUG=660198 Committed: https://crrev.com/8f240d5ca7cad065a2dd72fb819537914319c806 Cr-Commit-Position: refs/heads/master@{#428882} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8f240d5ca7cad065a2dd72fb819537914319c806 Cr-Commit-Position: refs/heads/master@{#428882} |
