Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2671613003:
Add checked_cast to size_t conversion in multibuffer_data_source.cc (Closed)
DescriptionAdd checked_cast to size_t conversion in multibuffer_data_source.cc
/analyze on Windows complains about the size_t conversion, so add a
checked_cast call to do the conversion. Memory usage is limited to 4Gb,
so this should never fail.
BUG=687394, 427616
TEST=compiles
Review-Url: https://codereview.chromium.org/2671613003
Cr-Commit-Position: refs/heads/master@{#447855}
Committed: https://chromium.googlesource.com/chromium/src/+/c88b8d3330e3b647b4e2b266bd9ffc53083e01e7
Patch Set 1 #
Total comments: 2
Messages
Total messages: 14 (7 generated)
Description was changed from ========== Add checked_cast to size_t conversion /analyze on Windows complains about the size_t conversion, so add a checked_cast call to do the conversion. Memory usage is limited to 4Gb, so this should never fail. BUG=687394 TEST=compiles ========== to ========== Add checked_cast to size_t conversion in multibuffer_data_source.cc /analyze on Windows complains about the size_t conversion, so add a checked_cast call to do the conversion. Memory usage is limited to 4Gb, so this should never fail. BUG=687394 TEST=compiles ==========
jrummell@chromium.org changed reviewers: + hubbe@chromium.org
Trivial change. PTAL.
lgtm
The CQ bit was checked by jrummell@chromium.org
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": 1, "attempt_start_ts": 1486065874032450, "parent_rev":
"06f6cd0653d33dc0e087cf3aede5df399ffc9db0", "commit_rev":
"c88b8d3330e3b647b4e2b266bd9ffc53083e01e7"}
Message was sent while issue was closed.
Description was changed from ========== Add checked_cast to size_t conversion in multibuffer_data_source.cc /analyze on Windows complains about the size_t conversion, so add a checked_cast call to do the conversion. Memory usage is limited to 4Gb, so this should never fail. BUG=687394 TEST=compiles ========== to ========== Add checked_cast to size_t conversion in multibuffer_data_source.cc /analyze on Windows complains about the size_t conversion, so add a checked_cast call to do the conversion. Memory usage is limited to 4Gb, so this should never fail. BUG=687394 TEST=compiles Review-Url: https://codereview.chromium.org/2671613003 Cr-Commit-Position: refs/heads/master@{#447855} Committed: https://chromium.googlesource.com/chromium/src/+/c88b8d3330e3b647b4e2b266bd9f... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/c88b8d3330e3b647b4e2b266bd9f...
Message was sent while issue was closed.
Description was changed from ========== Add checked_cast to size_t conversion in multibuffer_data_source.cc /analyze on Windows complains about the size_t conversion, so add a checked_cast call to do the conversion. Memory usage is limited to 4Gb, so this should never fail. BUG=687394 TEST=compiles Review-Url: https://codereview.chromium.org/2671613003 Cr-Commit-Position: refs/heads/master@{#447855} Committed: https://chromium.googlesource.com/chromium/src/+/c88b8d3330e3b647b4e2b266bd9f... ========== to ========== Add checked_cast to size_t conversion in multibuffer_data_source.cc /analyze on Windows complains about the size_t conversion, so add a checked_cast call to do the conversion. Memory usage is limited to 4Gb, so this should never fail. BUG=687394,427616 TEST=compiles Review-Url: https://codereview.chromium.org/2671613003 Cr-Commit-Position: refs/heads/master@{#447855} Committed: https://chromium.googlesource.com/chromium/src/+/c88b8d3330e3b647b4e2b266bd9f... ==========
Message was sent while issue was closed.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
Message was sent while issue was closed.
lgtm to me with the side note that a non-checked cast should also be fine, and that you should never have "stop /analyze from complaining" as your main reason to make a change. Only do it if it also makes sense (which I think this does). https://codereview.chromium.org/2671613003/diff/1/media/blink/multibuffer_dat... File media/blink/multibuffer_data_source.cc (right): https://codereview.chromium.org/2671613003/diff/1/media/blink/multibuffer_dat... media/blink/multibuffer_data_source.cc:339: return base::checked_cast<int64_t>(url_data_->CachedSize()) Why do we need a checked cast for converting to a larger size? The cast makes sense, I'm only curious about why a checked_cast was felt to be necessary. But, checked_cast is probably smart enough to be free in this case so it doesn't really matter.
Message was sent while issue was closed.
Aside: the comment talks about size_t but the code actually uses int64_t.
Message was sent while issue was closed.
Comments only. > lgtm to me with the side note that a non-checked cast should also be fine, and > that you should never have "stop /analyze from complaining" as your main reason > to make a change. Only do it if it also makes sense (which I think this does). Agreed. It does have the potential to generate the wrong result, but currently won't. However, just in case somebody down the road decides that much bigger caches are the thing to do, this will avoid the problem. > Aside: the comment talks about size_t but the code actually uses int64_t. I was trying to note that the implicit cast from size_t to int64_t done on the result now uses checked_cast on the intermediate result. Not sure what to say differently. "Add checked_cast to avoid possibly wrong implicit size_t cast to int64_t."? https://codereview.chromium.org/2671613003/diff/1/media/blink/multibuffer_dat... File media/blink/multibuffer_data_source.cc (right): https://codereview.chromium.org/2671613003/diff/1/media/blink/multibuffer_dat... media/blink/multibuffer_data_source.cc:339: return base::checked_cast<int64_t>(url_data_->CachedSize()) On 2017/02/02 22:22:52, brucedawson (slow) wrote: > Why do we need a checked cast for converting to a larger size? The cast makes > sense, I'm only curious about why a checked_cast was felt to be necessary. > > But, checked_cast is probably smart enough to be free in this case so it doesn't > really matter. My understanding is that on some systems size_t is 64 bits, so a simple conversion is bad. I thought Chromium encouraged use of the helpers, as past CL's have had comments that I should use them. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
