Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
262703002:
add default impl for context methods on shader (Closed)
|
Created:
6 years, 7 months ago by reed1 Modified:
6 years, 7 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
Descriptionadd default impl for context methods on shader
These are reasonable return values, since both of these methods can return a known value (0)
which means that no context can be created. This also makes it easier for chrome's subclasses
which already do not want to create a context, but having them actually overridden makes
changing the virtual signatures much harder.
BUG=skia:
Committed: http://code.google.com/p/skia/source/detail?r=14491
Patch Set 1 #
Total comments: 2
Patch Set 2 : update dox, keep contextSize() defaulting to 0 #
Total comments: 2
Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h File include/core/SkShader.h (right): https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h#newc... include/core/SkShader.h:221: * Base class implementation returns 0. Due to a requirement in SkSmallAllocator (which could be changed), returning 0 will fire an assert. If a class deliberately returns NULL for createContext, they should return sizeof(Context) here. https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h#newc... include/core/SkShader.h:223: virtual size_t contextSize() const; I like pure virtual function, because if someone writes a shader and overrides createContext but forgets to override contextSize, they will fail to compile, rather than get mysterious errors later.
On 2014/04/30 18:52:02, scroggo wrote: > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h > File include/core/SkShader.h (right): > > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h#newc... > include/core/SkShader.h:221: * Base class implementation returns 0. > Due to a requirement in SkSmallAllocator (which could be changed), returning 0 > will fire an assert. If a class deliberately returns NULL for createContext, > they should return sizeof(Context) here. Seems like asserting on a returned 0 is a good thing, no? > > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h#newc... > include/core/SkShader.h:223: virtual size_t contextSize() const; > I like pure virtual function, because if someone writes a shader and overrides > createContext but forgets to override contextSize, they will fail to compile, > rather than get mysterious errors later. Hmm, I don't see as much value. They can clearly override those methods and return just what I'm returning. And then they still have to debug their code.
On 2014/04/30 18:56:45, reed1 wrote: > On 2014/04/30 18:52:02, scroggo wrote: > > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h > > File include/core/SkShader.h (right): > > > > > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h#newc... > > include/core/SkShader.h:221: * Base class implementation returns 0. > > Due to a requirement in SkSmallAllocator (which could be changed), returning 0 > > will fire an assert. If a class deliberately returns NULL for createContext, > > they should return sizeof(Context) here. > > Seems like asserting on a returned 0 is a good thing, no? Not necessarily. They may deliberately return NULL, like SkEmptyShader. In that case, we don't want to assert. I suppose the counter argument here is to insist they override contextSize like SkEmptyShader does. I still don't like that the assert happens downstream. What if this function asserted itself, along with comments that if the subclass wants to return NULL for createContext() then contextSize() needs to return >= sizeof(Context)? > > > > > > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h#newc... > > include/core/SkShader.h:223: virtual size_t contextSize() const; > > I like pure virtual function, because if someone writes a shader and overrides > > createContext but forgets to override contextSize, they will fail to compile, > > rather than get mysterious errors later. > > Hmm, I don't see as much value. They can clearly override those methods and > return just what I'm returning. And then they still have to debug their code. Sure, but a good API makes it hard to make mistakes. Maybe what we really need is an explicit comment in the declaration of createContext that if the subclass overrides this function it must also override contextSize (previously it was obvious because they were both pure virtual).
On 2014/04/30 19:10:59, scroggo wrote: > On 2014/04/30 18:56:45, reed1 wrote: > > On 2014/04/30 18:52:02, scroggo wrote: > > > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h > > > File include/core/SkShader.h (right): > > > > > > > > > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h#newc... > > > include/core/SkShader.h:221: * Base class implementation returns 0. > > > Due to a requirement in SkSmallAllocator (which could be changed), returning > 0 > > > will fire an assert. If a class deliberately returns NULL for createContext, > > > they should return sizeof(Context) here. > > > > Seems like asserting on a returned 0 is a good thing, no? > > Not necessarily. They may deliberately return NULL, like SkEmptyShader. In that > case, we > don't want to assert. I suppose the counter argument here is to insist they > override > contextSize like SkEmptyShader does. > > I still don't like that the assert happens downstream. What if this function > asserted > itself, along with comments that if the subclass wants to return NULL for > createContext() > then contextSize() needs to return >= sizeof(Context)? > > > > > > > > > > > > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h#newc... > > > include/core/SkShader.h:223: virtual size_t contextSize() const; > > > I like pure virtual function, because if someone writes a shader and > overrides > > > createContext but forgets to override contextSize, they will fail to > compile, > > > rather than get mysterious errors later. > > > > Hmm, I don't see as much value. They can clearly override those methods and > > return just what I'm returning. And then they still have to debug their code. > > Sure, but a good API makes it hard to make mistakes. Maybe what we really need > is an > explicit comment in the declaration of createContext that if the subclass > overrides > this function it must also override contextSize (previously it was obvious > because > they were both pure virtual). I'm fine to change the default to sizeof(Context)
On 2014/04/30 19:21:58, reed1 wrote: > On 2014/04/30 19:10:59, scroggo wrote: > > On 2014/04/30 18:56:45, reed1 wrote: > > > On 2014/04/30 18:52:02, scroggo wrote: > > > > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h > > > > File include/core/SkShader.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h#newc... > > > > include/core/SkShader.h:221: * Base class implementation returns 0. > > > > Due to a requirement in SkSmallAllocator (which could be changed), > returning > > 0 > > > > will fire an assert. If a class deliberately returns NULL for > createContext, > > > > they should return sizeof(Context) here. > > > > > > Seems like asserting on a returned 0 is a good thing, no? > > > > Not necessarily. They may deliberately return NULL, like SkEmptyShader. In > that > > case, we > > don't want to assert. I suppose the counter argument here is to insist they > > override > > contextSize like SkEmptyShader does. > > > > I still don't like that the assert happens downstream. What if this function > > asserted > > itself, along with comments that if the subclass wants to return NULL for > > createContext() > > then contextSize() needs to return >= sizeof(Context)? > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/262703002/diff/1/include/core/SkShader.h#newc... > > > > include/core/SkShader.h:223: virtual size_t contextSize() const; > > > > I like pure virtual function, because if someone writes a shader and > > overrides > > > > createContext but forgets to override contextSize, they will fail to > > compile, > > > > rather than get mysterious errors later. > > > > > > Hmm, I don't see as much value. They can clearly override those methods and > > > return just what I'm returning. And then they still have to debug their > code. > > > > Sure, but a good API makes it hard to make mistakes. Maybe what we really need > > is an > > explicit comment in the declaration of createContext that if the subclass > > overrides > > this function it must also override contextSize (previously it was obvious > > because > > they were both pure virtual). > > I'm fine to change the default to sizeof(Context) Although I hinted at that in #1, now that I think about it, I don't think it's such a great idea. I think it'd be more helpful to follow my comments in #4. Changing the default to sizeof(Context) has the same danger as 0 - the bug shows up later (even worse in this case because it won't necessarily be an assert - we may have tried to write their large context somewhere it doesn't fit, writing into (and later reading from) random memory).
updated dox, kept the def contextSize at 0 ptal
On 2014/04/30 21:18:32, reed1 wrote: > updated dox, kept the def contextSize at 0 > > ptal LGTM
I like the comments. If we want pixel_ref_utils_unittest.cc to work without asserting, we may need to return sizeof(Context). lgtm with or without that change. https://codereview.chromium.org/262703002/diff/20001/src/core/SkShader.cpp File src/core/SkShader.cpp (right): https://codereview.chromium.org/262703002/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:72: return 0; This may assert in skia/ext/pixel_ref_utils_unittest.cc after https://codereview.chromium.org/264463003/
The CQ bit was checked by reed@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/reed@google.com/262703002/20001
Message was sent while issue was closed.
Change committed as 14491
Message was sent while issue was closed.
https://codereview.chromium.org/262703002/diff/20001/src/core/SkShader.cpp File src/core/SkShader.cpp (right): https://codereview.chromium.org/262703002/diff/20001/src/core/SkShader.cpp#ne... src/core/SkShader.cpp:72: return 0; On 2014/04/30 21:38:23, scroggo wrote: > This may assert in skia/ext/pixel_ref_utils_unittest.cc after > https://codereview.chromium.org/264463003/ Yes, this will fail if a subclass doesn't overwrite this method. In [1] we'd pass 0 as the allocation size to SkSmallAllocator which requires that the allocation size is at least the size of T (= SkShader::Context). Could we return 'sizeof(SkShader::Context)' instead? Otherwise we'd have to check for 'contextSize() == 0' before trying to allocate. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
