Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1911363002:
SkMD5: cleanup header and minor refactor (Closed)
|
Created:
4 years, 8 months ago by hal.canary Modified:
4 years, 8 months ago Reviewers:
tomhudson CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkMD5: cleanup header and minor refactor
Also: I now define a non-virtual function in terms of a final virtual
function. This reduces the number of actual functions while adding
no overhead.
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1911363002
Committed: https://skia.googlesource.com/skia/+/272aa12c63e32c2845cbb5549660c08c31159282
Patch Set 1 #
Total comments: 6
Patch Set 2 : 2016-04-22 (Friday) 10:10:53 EDT #Patch Set 3 : 2016-04-22 (Friday) 10:13:05 EDT #Patch Set 4 : 2016-04-22 (Friday) 10:53:13 EDT #
Messages
Total messages: 24 (13 generated)
Description was changed from ========== SkMD5: cleanup header and minor refactor Also: I now define a non-virtual function in terms of a final virtual function. This reduces the number of actual functions while adding no overhead. ========== to ========== SkMD5: cleanup header and minor refactor Also: I now define a non-virtual function in terms of a final virtual function. This reduces the number of actual functions while adding no overhead. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911363002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911363002/1
halcanary@google.com changed reviewers: + tomhudson@google.com
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.cpp File src/core/SkMD5.cpp (right): https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.cpp#newcode39 src/core/SkMD5.cpp:39: // Note that this treats the buffer as a series of uint8_t values. Not sure this comment is helpful - it's sitting here right next to the code that does the reinterpretation, which isn't very complex. Why should the user care? Who needs to know? Maybe this (or something like it - not sure why the reinterpret is really important enough to matter) belongs in the header, or maybe it belongs in /dev/null? https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.h File src/core/SkMD5.h (left): https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.h#oldcode11 src/core/SkMD5.h:11: #include "SkTypes.h" Trusting on getting SkTypes via SkStream? We call SkToSizeT(), let's include-what-we-use. https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.h File src/core/SkMD5.h (right): https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.h#newcode19 src/core/SkMD5.h:19: Calling this after finish is undefined. */ But not so undefined that we should be returning false?
https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.cpp File src/core/SkMD5.cpp (right): https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.cpp#newcode39 src/core/SkMD5.cpp:39: // Note that this treats the buffer as a series of uint8_t values. On 2016/04/22 13:35:59, tomhudson wrote: > Not sure this comment is helpful - it's sitting here right next to the code that > does the reinterpretation, which isn't very complex. Why should the user care? > Who needs to know? > > Maybe this (or something like it - not sure why the reinterpret is really > important enough to matter) belongs in the header, or maybe it belongs in > /dev/null? Done. https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.h File src/core/SkMD5.h (left): https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.h#oldcode11 src/core/SkMD5.h:11: #include "SkTypes.h" On 2016/04/22 13:35:59, tomhudson wrote: > Trusting on getting SkTypes via SkStream? > We call SkToSizeT(), let's include-what-we-use. Acknowledged. https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.h File src/core/SkMD5.h (right): https://codereview.chromium.org/1911363002/diff/1/src/core/SkMD5.h#newcode19 src/core/SkMD5.h:19: Calling this after finish is undefined. */ On 2016/04/22 13:35:59, tomhudson wrote: > But not so undefined that we should be returning false? we do not now track that state.
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911363002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911363002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Description was changed from ========== SkMD5: cleanup header and minor refactor Also: I now define a non-virtual function in terms of a final virtual function. This reduces the number of actual functions while adding no overhead. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkMD5: cleanup header and minor refactor Also: I now define a non-virtual function in terms of a final virtual function. This reduces the number of actual functions while adding no overhead. COMMIT=false GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com Link to the patchset: https://codereview.chromium.org/1911363002/#ps60001 (title: "2016-04-22 (Friday) 10:53:13 EDT")
The CQ bit was unchecked by commit-bot@chromium.org
COMMIT=false detected. CQ is abandoning the patch.
Description was changed from ========== SkMD5: cleanup header and minor refactor Also: I now define a non-virtual function in terms of a final virtual function. This reduces the number of actual functions while adding no overhead. COMMIT=false GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkMD5: cleanup header and minor refactor Also: I now define a non-virtual function in terms of a final virtual function. This reduces the number of actual functions while adding no overhead. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911363002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911363002/60001
Message was sent while issue was closed.
Description was changed from ========== SkMD5: cleanup header and minor refactor Also: I now define a non-virtual function in terms of a final virtual function. This reduces the number of actual functions while adding no overhead. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkMD5: cleanup header and minor refactor Also: I now define a non-virtual function in terms of a final virtual function. This reduces the number of actual functions while adding no overhead. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/272aa12c63e32c2845cbb5549660c08c31159282 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/272aa12c63e32c2845cbb5549660c08c31159282 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
