Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1604253002:
SkValue: implementation, unit test (Closed)
|
Created:
4 years, 11 months ago by hal.canary Modified:
4 years, 11 months ago Reviewers:
mtklein CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkValue: implementation, unit test
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1604253002
Committed: https://skia.googlesource.com/skia/+/76097f823598076a440ed9dc7fa6611583308002
Patch Set 1 #Patch Set 2 : 2016-01-20 (Wednesday) 07:22:07 EST #
Total comments: 20
Patch Set 3 : 2016-01-20 (Wednesday) 10:14:15 EST #Patch Set 4 : stray whitespce remvd #
Total comments: 4
Patch Set 5 : simplification #Patch Set 6 : comments from mtklein #Patch Set 7 : another unit test #
Messages
Total messages: 22 (9 generated)
Description was changed from ========== SkValue: implementation, unit test ========== to ========== SkValue: implementation, unit test GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
halcanary@google.com changed reviewers: + mtklein@google.com
PTAL
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/1604253002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1604253002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp File src/core/SkValue.cpp (right): https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:38: return fVec.at(index); vector::at does bounds checks. I think you want fVec[index]. https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:163: #define FN(NAME, MNAME, TYPE) \ Seems like we can get most of the concision by using templates instead of macros? I don't mind using macros when needed, but we're not #quoting any terms or doing anything that really requires it here. template <typename T> /*private, static*/ SkValue SkValue::FromTs(SkValue::Type type, SkData* data) { SkValue val; val.fType = type; val.fBytes = SkRef(data); SkASSERT(val.isData()); SkASSERT(0 == (reinterpret_cast<uintptr_t>(data->bytes()) & (sizeof(T)-1)); return val; } SkValue SkValue::FromU16s(SkData* data) { return FromTs<uint16_t>(U16s, data); } SkValue SkValue::FromU32s(SkData* data) { return FromTs<uint32_t>(U16s, data); } SkValue SkValue::FromF32s(SkData* data) { return FromTs< float>(F32s, data); } template <typename T> /*private*/ SkValue SkValue::ts(SkValue::Type type, int* count) const { SkASSERT(fType == type); if (count) { *count = fBytes->size() / sizeof(T); } return (const T*)fBytes->data(); } const uint16_t* SkValue::u16s(int* count) const { return this->ts<uint16_t>(U16s, count); } const uint32_t* SkValue::u32s(int* count) const { return this->ts<uint32_t>(U32s, count); } const float* SkValue::f32s(int* count) const { return this->ts<float>(F32s, count); } This could be even more concise if we make a compile time mapping back and forth between type and Type. https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:181: #undef FN #undef PTR_IS_ALIGNED_TO also, or inline it into FN? https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:189: val.set((SkValue::Key)i, SkValue::FromF32(mat[i])); val.set(i, ...) ? Isn't this cast implicit? https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:203: if (key < 9) { the indentation from 203-211 is pretty weird https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:208: (*m)[(int)key] = v.f32(); (*m)[key] ? Isn't this cast implicit? https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:210: } I guess, } else { good = false; } here too? https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.h File src/core/SkValue.h (right): https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.h#newc... src/core/SkValue.h:69: bool isData() const { private? https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.h#newc... src/core/SkValue.h:77: bool isObject() const { return fType > kMaxBuiltin; } private? https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.h#newc... src/core/SkValue.h:86: template <typename T> static SkValue Encode(const T&); Let's make these template <typename T> SkValue SkToValue(const T&); template <typename T> bool SkFromValue(const SkValue&, T*); It'll force us to stay honest to restrict these functions to use only SkValue's public API. Encode/Decode sound like we're doing something interesting, and I'd like these functions to sound more mundane to reflect their (hopefully) mundane nature. I am not married on To/From.
Great. I think we're on the same page. https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp File src/core/SkValue.cpp (right): https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:38: return fVec.at(index); On 2016/01/20 at 14:20:58, mtklein wrote: > vector::at does bounds checks. I think you want fVec[index]. done https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:163: #define FN(NAME, MNAME, TYPE) \ On 2016/01/20 at 14:20:58, mtklein wrote: > Seems like we can get most of the concision by using templates instead of macros? I don't mind using macros when needed, but we're not #quoting any terms or doing anything that really requires it here. > > template <typename T> > /*private, static*/ SkValue SkValue::FromTs(SkValue::Type type, SkData* data) { > SkValue val; > val.fType = type; > val.fBytes = SkRef(data); > > SkASSERT(val.isData()); > SkASSERT(0 == (reinterpret_cast<uintptr_t>(data->bytes()) & (sizeof(T)-1)); > return val; > } > > SkValue SkValue::FromU16s(SkData* data) { return FromTs<uint16_t>(U16s, data); } > SkValue SkValue::FromU32s(SkData* data) { return FromTs<uint32_t>(U16s, data); } > SkValue SkValue::FromF32s(SkData* data) { return FromTs< float>(F32s, data); } > > template <typename T> > /*private*/ SkValue SkValue::ts(SkValue::Type type, int* count) const { > SkASSERT(fType == type); > if (count) { *count = fBytes->size() / sizeof(T); } > return (const T*)fBytes->data(); > } > > const uint16_t* SkValue::u16s(int* count) const { return this->ts<uint16_t>(U16s, count); } > const uint32_t* SkValue::u32s(int* count) const { return this->ts<uint32_t>(U32s, count); } > const float* SkValue::f32s(int* count) const { return this->ts<float>(F32s, count); } > > This could be even more concise if we make a compile time mapping back and forth between type and Type. DONE. awesome. https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:181: #undef FN On 2016/01/20 at 14:20:58, mtklein wrote: > #undef PTR_IS_ALIGNED_TO also, or inline it into FN? done https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:189: val.set((SkValue::Key)i, SkValue::FromF32(mat[i])); On 2016/01/20 at 14:20:59, mtklein wrote: > val.set(i, ...) ? Isn't this cast implicit? done. https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:203: if (key < 9) { On 2016/01/20 at 14:20:59, mtklein wrote: > the indentation from 203-211 is pretty weird let me write that simpler. https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:208: (*m)[(int)key] = v.f32(); On 2016/01/20 at 14:20:58, mtklein wrote: > (*m)[key] ? Isn't this cast implicit? done. https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:210: } On 2016/01/20 at 14:20:59, mtklein wrote: > I guess, > > } else { > good = false; > } > > here too? right. https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.h File src/core/SkValue.h (right): https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.h#newc... src/core/SkValue.h:69: bool isData() const { On 2016/01/20 at 14:20:59, mtklein wrote: > private? done https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.h#newc... src/core/SkValue.h:77: bool isObject() const { return fType > kMaxBuiltin; } On 2016/01/20 at 14:20:59, mtklein wrote: > private? done https://codereview.chromium.org/1604253002/diff/20001/src/core/SkValue.h#newc... src/core/SkValue.h:86: template <typename T> static SkValue Encode(const T&); On 2016/01/20 at 14:20:59, mtklein wrote: > Let's make these > > template <typename T> > SkValue SkToValue(const T&); > > template <typename T> > bool SkFromValue(const SkValue&, T*); > > It'll force us to stay honest to restrict these functions to use only SkValue's public API. > > Encode/Decode sound like we're doing something interesting, and I'd like these functions to sound more mundane to reflect their (hopefully) mundane nature. I am not married on To/From. done.
https://codereview.chromium.org/1604253002/diff/60001/src/core/SkValue.cpp File src/core/SkValue.cpp (right): https://codereview.chromium.org/1604253002/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:161: const T* SkValue::AsArray(SkValue::Type t, int* count) const { I'm getting confused by AsArray, Array, ValueArray. This is not static, so its name should start lower case. asTs to match FromTs? https://codereview.chromium.org/1604253002/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:162: SkASSERT(t == fType); merge related SkASSERT(t == fType && this->isData()) ?
The CQ bit was checked by mtklein@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/1604253002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1604253002/60001
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 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/1604253002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1604253002/80001
https://codereview.chromium.org/1604253002/diff/60001/src/core/SkValue.cpp File src/core/SkValue.cpp (right): https://codereview.chromium.org/1604253002/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:161: const T* SkValue::AsArray(SkValue::Type t, int* count) const { On 2016/01/20 at 15:47:50, mtklein wrote: > I'm getting confused by AsArray, Array, ValueArray. > > This is not static, so its name should start lower case. > > asTs to match FromTs? Righto. Done. What should ValueArray be named? I don't know. CreateArray? https://codereview.chromium.org/1604253002/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:162: SkASSERT(t == fType); On 2016/01/20 at 15:47:50, mtklein wrote: > merge related SkASSERT(t == fType && this->isData()) ? Done.
The CQ bit was checked by mtklein@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1604253002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1604253002/120001
Message was sent while issue was closed.
Description was changed from ========== SkValue: implementation, unit test GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkValue: implementation, unit test GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/76097f823598076a440ed9dc7fa6611583308002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/76097f823598076a440ed9dc7fa6611583308002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
