Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2084213003:
Make container classes in SkTemplates.h more consistent (Closed)
|
Created:
4 years, 6 months ago by csmartdalton Modified:
4 years, 6 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionMake container classes in SkTemplates.h more consistent
Ensures that ".get()" always returns null when a container is empty.
Also ensures consistent assert behavior for array counts.
There are still differences in that the malloc variants take a size_t
and the arrays take an int, and that SkAutoSTMalloc defaults to the
stack-allocated buffer wheras the other containers default to null.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084213003
Committed: https://skia.googlesource.com/skia/+/d0e402f99949127624b5c5e6d673a44a71940489
Patch Set 1 #
Total comments: 6
Patch Set 2 : Make buffer classes in SkTemplates.h more consistent #Patch Set 3 : Make buffer classes in SkTemplates.h more consistent #
Total comments: 2
Patch Set 4 : rebase #Patch Set 5 : comments #Patch Set 6 : Make container classes in SkTemplates.h more consistent #Patch Set 7 : Make container classes in SkTemplates.h more consistent #Patch Set 8 : Make container classes in SkTemplates.h more consistent #Patch Set 9 : break dependency #Messages
Total messages: 26 (11 generated)
Description was changed from ========== Make SkAutoSTMalloc null when count is 0 This is useful for asserts and identifying bugs. BUG=skia: ========== to ========== Make SkAutoSTMalloc null when count is 0 This is useful for asserts and identifying bugs. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084213003 ==========
csmartdalton@google.com changed reviewers: + bsalomon@google.com, mtklein@google.com
What do you guys think of this change?
reed@google.com changed reviewers: + reed@google.com
neutral -- if we're emulating/wrapping malloc, then null may not be as correct, since malloc would return non-null (I think), but I don't feel strongly. https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates.h File include/private/SkTemplates.h (right): https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates... include/private/SkTemplates.h:332: SkASSERT(0 == count); Can this ever not be true?
On 2016/06/22 11:33:30, reed1 wrote: > neutral -- if we're emulating/wrapping malloc, then null may not be as correct, > since malloc would return non-null (I think), but I don't feel strongly. SkAutoTMalloc::reset also makes itself null when the count is 0. I just thought this might be nice for asserting a buffer is empty (i.e. documenting the conditions in code when it will be empty before using it).
I'm also neutral on this. Maybe you could show a use case? https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates.h File include/private/SkTemplates.h (right): https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates... include/private/SkTemplates.h:378: void realloc(size_t count) { Wouldn't this need to be updated, too?
On 2016/06/22 13:30:29, bsalomon wrote: > I'm also neutral on this. Maybe you could show a use case? > > https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates.h > File include/private/SkTemplates.h (right): > > https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates... > include/private/SkTemplates.h:378: void realloc(size_t count) { > Wouldn't this need to be updated, too? An example would be here, line 214: https://codereview.chromium.org/2066993003/diff/120001/src/gpu/instanced/GLIn... fGLDrawCmdsInfo is a buffer that only gets allocated if EXT_base_instance is not supported. The point of the assert is mostly to say "Don't try to use this if you have EXT_base_instance because it will be empty." I don't have a strong preference. If we would rather not submit this change I can remove the assert instead.
Description was changed from ========== Make SkAutoSTMalloc null when count is 0 This is useful for asserts and identifying bugs. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084213003 ========== to ========== Make buffer classes in SkTemplates.h more consistent Ensures that ".get()" always returns nullptr when a buffer is empty. Also ensures consistent assert behavior for array counts. There are still differences in that the malloc variants take a size_t and the arrays take an int, and that SkAutoSTMalloc defaults to the stack-allocated buffer wheras the other buffers default to null. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084213003 ==========
Make the changes as discussed earlier this morning. https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates.h File include/private/SkTemplates.h (right): https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates... include/private/SkTemplates.h:332: SkASSERT(0 == count); On 2016/06/22 11:33:30, reed1 wrote: > Can this ever not be true? The idea would be that if somebody unintentionally calls this with a negative number it helps them find their bug. Of course, it's already making the pointer null, which would bring it to their attention pretty quickly anyway.
Make the changes as discussed earlier this morning.
Make the changes as discussed earlier this morning.
https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates.h File include/private/SkTemplates.h (right): https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates... include/private/SkTemplates.h:332: SkASSERT(0 == count); On 2016/06/23 17:07:46, csmartdalton wrote: > On 2016/06/22 11:33:30, reed1 wrote: > > Can this ever not be true? > > The idea would be that if somebody unintentionally calls this with a negative > number it helps them find their bug. Of course, it's already making the pointer > null, which would bring it to their attention pretty quickly anyway. but count is size_t, which will never appear to be negative
lgtm https://codereview.chromium.org/2084213003/diff/40001/include/private/SkTempl... File include/private/SkTemplates.h (right): https://codereview.chromium.org/2084213003/diff/40001/include/private/SkTempl... include/private/SkTemplates.h:212: SkASSERT(0 == count); I think it'd be slightly clearer to just put SkASSERT(count >= 0); at the top of the function.
https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates.h File include/private/SkTemplates.h (right): https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates... include/private/SkTemplates.h:332: SkASSERT(0 == count); On 2016/06/23 17:44:36, reed1 wrote: > On 2016/06/23 17:07:46, csmartdalton wrote: > > On 2016/06/22 11:33:30, reed1 wrote: > > > Can this ever not be true? > > > > The idea would be that if somebody unintentionally calls this with a negative > > number it helps them find their bug. Of course, it's already making the > pointer > > null, which would bring it to their attention pretty quickly anyway. > > but count is size_t, which will never appear to be negative Yeah, I realized it was size_t after the fact :) https://codereview.chromium.org/2084213003/diff/1/include/private/SkTemplates... include/private/SkTemplates.h:378: void realloc(size_t count) { On 2016/06/22 13:30:29, bsalomon wrote: > Wouldn't this need to be updated, too? Updated to what? https://codereview.chromium.org/2084213003/diff/40001/include/private/SkTempl... File include/private/SkTemplates.h (right): https://codereview.chromium.org/2084213003/diff/40001/include/private/SkTempl... include/private/SkTemplates.h:212: SkASSERT(0 == count); On 2016/06/23 17:53:34, bsalomon wrote: > I think it'd be slightly clearer to just put SkASSERT(count >= 0); at the top of > the function. Done.
lgtm
Description was changed from ========== Make buffer classes in SkTemplates.h more consistent Ensures that ".get()" always returns nullptr when a buffer is empty. Also ensures consistent assert behavior for array counts. There are still differences in that the malloc variants take a size_t and the arrays take an int, and that SkAutoSTMalloc defaults to the stack-allocated buffer wheras the other buffers default to null. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084213003 ========== to ========== Make container classes in SkTemplates.h more consistent Ensures that ".get()" always returns null when a container is empty. Also ensures consistent assert behavior for array counts. There are still differences in that the malloc variants take a size_t and the arrays take an int, and that SkAutoSTMalloc defaults to the stack-allocated buffer wheras the other containers default to null. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084213003 ==========
The CQ bit was checked by csmartdalton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/2084213003/#ps130001 (title: "Make container classes in SkTemplates.h more consistent")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2085043002 Patch 100001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by csmartdalton@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/2084213003/#ps150001 (title: "break dependency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2084213003/150001
Message was sent while issue was closed.
Description was changed from ========== Make container classes in SkTemplates.h more consistent Ensures that ".get()" always returns null when a container is empty. Also ensures consistent assert behavior for array counts. There are still differences in that the malloc variants take a size_t and the arrays take an int, and that SkAutoSTMalloc defaults to the stack-allocated buffer wheras the other containers default to null. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084213003 ========== to ========== Make container classes in SkTemplates.h more consistent Ensures that ".get()" always returns null when a container is empty. Also ensures consistent assert behavior for array counts. There are still differences in that the malloc variants take a size_t and the arrays take an int, and that SkAutoSTMalloc defaults to the stack-allocated buffer wheras the other containers default to null. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2084213003 Committed: https://skia.googlesource.com/skia/+/d0e402f99949127624b5c5e6d673a44a71940489 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:150001) as https://skia.googlesource.com/skia/+/d0e402f99949127624b5c5e6d673a44a71940489 |
