Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1146903004:
SkBlockMemoryStream implements peek() (Closed)
|
Created:
5 years, 7 months ago by hal.canary Modified:
5 years, 7 months ago Reviewers:
scroggo CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkBlockMemoryStream implements peek()
Committed: https://skia.googlesource.com/skia/+/e797d0de67a5dd4b7b0b66dbb8b33be46b74240e
Patch Set 1 #
Total comments: 10
Patch Set 2 : 2015-05-21 (Thursday) 09:27:41 EDT #
Total comments: 2
Patch Set 3 : 2015-05-21 (Thursday) 10:35:39 EDT #Patch Set 4 : 2015-05-21 (Thursday) 10:41:51 EDT #Patch Set 5 : 2015-05-21 (Thursday) 11:06:57 EDT #Messages
Total messages: 20 (9 generated)
halcanary@google.com changed reviewers: + scroggo@google.com
PTAL
https://codereview.chromium.org/1146903004/diff/1/src/core/SkStream.cpp File src/core/SkStream.cpp (right): https://codereview.chromium.org/1146903004/diff/1/src/core/SkStream.cpp#newco... src/core/SkStream.cpp:731: if (!buff) { In other implementations, we SkASSERT(buff). They should all behave the same (the documentation also states that buff must not be null). I don't understand why the caller would want to get a return value of true if they passed a null buffer (it seems like they made a mistake if they passed null). https://codereview.chromium.org/1146903004/diff/1/tests/StreamTest.cpp File tests/StreamTest.cpp (right): https://codereview.chromium.org/1146903004/diff/1/tests/StreamTest.cpp#newcod... tests/StreamTest.cpp:289: size_t size = (rand.nextU() % sizeof(buffer)) + 1; Why not rand.nextRangeU(1, sizeof(buffer))? https://codereview.chromium.org/1146903004/diff/1/tests/StreamTest.cpp#newcod... tests/StreamTest.cpp:291: if (size > 0) { Can size ever be 0? I guess with an empty asset? It does not look like we ever create one. (Though if we did, it seems like this test would fail below when we try to read it.) https://codereview.chromium.org/1146903004/diff/1/tests/StreamTest.cpp#newcod... tests/StreamTest.cpp:303: asset->read(&value, 1); Check the return value too? https://codereview.chromium.org/1146903004/diff/1/tests/StreamTest.cpp#newcod... tests/StreamTest.cpp:329: valueSource.setSeed(kSeed); // reseed. So if I understand correctly, you're just trying to get the same random string of numbers to put in expected?
https://codereview.chromium.org/1146903004/diff/1/src/core/SkStream.cpp File src/core/SkStream.cpp (right): https://codereview.chromium.org/1146903004/diff/1/src/core/SkStream.cpp#newco... src/core/SkStream.cpp:731: if (!buff) { On 2015/05/20 15:43:35, scroggo wrote: > In other implementations, we SkASSERT(buff). They should all behave the same > (the documentation also states that buff must not be null). I don't understand > why the caller would want to get a return value of true if they passed a null > buffer (it seems like they made a mistake if they passed null). Done. https://codereview.chromium.org/1146903004/diff/1/tests/StreamTest.cpp File tests/StreamTest.cpp (right): https://codereview.chromium.org/1146903004/diff/1/tests/StreamTest.cpp#newcod... tests/StreamTest.cpp:289: size_t size = (rand.nextU() % sizeof(buffer)) + 1; On 2015/05/20 15:43:35, scroggo wrote: > Why not rand.nextRangeU(1, sizeof(buffer))? Done. https://codereview.chromium.org/1146903004/diff/1/tests/StreamTest.cpp#newcod... tests/StreamTest.cpp:291: if (size > 0) { On 2015/05/20 15:43:35, scroggo wrote: > Can size ever be 0? I guess with an empty asset? It does not look like we ever > create one. (Though if we did, it seems like this test would fail below when we > try to read it.) Done. Of course size is always > 0! https://codereview.chromium.org/1146903004/diff/1/tests/StreamTest.cpp#newcod... tests/StreamTest.cpp:303: asset->read(&value, 1); On 2015/05/20 15:43:35, scroggo wrote: > Check the return value too? Done. https://codereview.chromium.org/1146903004/diff/1/tests/StreamTest.cpp#newcod... tests/StreamTest.cpp:329: valueSource.setSeed(kSeed); // reseed. On 2015/05/20 15:43:35, scroggo wrote: > So if I understand correctly, you're just trying to get the same random string > of numbers to put in expected? Acknowledged.
lgtm https://codereview.chromium.org/1146903004/diff/20001/src/core/SkStream.cpp File src/core/SkStream.cpp (right): https://codereview.chromium.org/1146903004/diff/20001/src/core/SkStream.cpp#n... src/core/SkStream.cpp:733: const SkDynamicMemoryWStream::Block* current = fCurrent; Patch set 2 separates out the code for dealing with the current block from the code that deals with the rest. AFAICT, this means we skip the following (unnecessary) calculations for all loop iterations but the first (which technically is no longer an iteration of the loop): - subtract 0 - add 0 - assign 0 to a variable (this is removed from the first as well) On the other hand, we have some mostly duplicated code. Unless the original code is noticeably slower, I'd prefer less duplication.
https://codereview.chromium.org/1146903004/diff/20001/src/core/SkStream.cpp File src/core/SkStream.cpp (right): https://codereview.chromium.org/1146903004/diff/20001/src/core/SkStream.cpp#n... src/core/SkStream.cpp:733: const SkDynamicMemoryWStream::Block* current = fCurrent; On 2015/05/21 14:13:18, scroggo wrote: > On the other hand, we have some mostly duplicated code. Unless the original code > is noticeably slower, I'd prefer less duplication. reverted
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1146903004/#ps40001 (title: "2015-05-21 (Thursday) 10:35:39 EDT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146903004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1146903004/#ps60001 (title: "2015-05-21 (Thursday) 10:41:51 EDT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146903004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1146903004/#ps80001 (title: "2015-05-21 (Thursday) 11:06:57 EDT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1146903004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/e797d0de67a5dd4b7b0b66dbb8b33be46b74240e |
