Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1769093002:
add virtual ~Effect() (Closed)
Descriptionadd virtual ~Effect()
Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>.
But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl().
Making ~Effect() virtual fixes this.
This should make our Google3 tests sized-delete clean, unblocking those folks.
Committed: https://skia.googlesource.com/skia/+/71aca549914c868d50cd51ddff3fb7a5bfe250cd
Patch Set 1 #
Total comments: 2
Patch Set 2 : undo comment #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== add virtual ~Effect() Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>. But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl(). Making ~Effect() virtual fixes this. Also adds a note that it'd be nice to figure out how to statically assert this. This should make our Google3 tests sized-delete clean, unblocking those folks. BUG=skia: ========== to ========== add virtual ~Effect() Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>. But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl(). Making ~Effect() virtual fixes this. Also adds a note that it'd be nice to figure out how to statically assert this. This should make our Google3 tests sized-delete clean, unblocking those folks. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
mtklein@chromium.org changed reviewers: + bungeman@google.com
The CQ bit was checked by mtklein@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1769093002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769093002/1
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-03-07 23:47 UTC
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
Description was changed from ========== add virtual ~Effect() Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>. But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl(). Making ~Effect() virtual fixes this. Also adds a note that it'd be nice to figure out how to statically assert this. This should make our Google3 tests sized-delete clean, unblocking those folks. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== add virtual ~Effect() Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>. But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl(). Making ~Effect() virtual fixes this. Also adds a note that it'd be nice to figure out how to statically assert this. This should make our Google3 tests sized-delete clean, unblocking those folks. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... No public API changes. TBR=reed@google.com ==========
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/1769093002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769093002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1769093002/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1769093002/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:250: Putting this here is a bit over constrained. sk_sp doesn't call 'delete', it calls 'unref'. It sounds a bit dumb, but 'unref' in the base class could figure out what derived class it was, downcast, and then properly delete. I think it really is up to the individual class implementation to make sure that it's properly written in this regard.
Description was changed from ========== add virtual ~Effect() Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>. But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl(). Making ~Effect() virtual fixes this. Also adds a note that it'd be nice to figure out how to statically assert this. This should make our Google3 tests sized-delete clean, unblocking those folks. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... No public API changes. TBR=reed@google.com ========== to ========== add virtual ~Effect() Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>. But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl(). Making ~Effect() virtual fixes this. This should make our Google3 tests sized-delete clean, unblocking those folks. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1769093002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769093002/20001
https://codereview.chromium.org/1769093002/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1769093002/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:250: On 2016/03/07 at 19:29:45, bungeman1 wrote: > Putting this here is a bit over constrained. sk_sp doesn't call 'delete', it calls 'unref'. It sounds a bit dumb, but 'unref' in the base class could figure out what derived class it was, downcast, and then properly delete. I think it really is up to the individual class implementation to make sure that it's properly written in this regard. Agreed. Now gone.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== add virtual ~Effect() Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>. But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl(). Making ~Effect() virtual fixes this. This should make our Google3 tests sized-delete clean, unblocking those folks. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== add virtual ~Effect() Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>. But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl(). Making ~Effect() virtual fixes this. This should make our Google3 tests sized-delete clean, unblocking those folks. ==========
The CQ bit was checked by bungeman@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1769093002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769093002/20001
Message was sent while issue was closed.
Description was changed from ========== add virtual ~Effect() Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>. But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl(). Making ~Effect() virtual fixes this. This should make our Google3 tests sized-delete clean, unblocking those folks. ========== to ========== add virtual ~Effect() Today we make an sk_sp<Effect> from an sk_sp<EffectImpl>. But when the sk_sp<Effect> dies, it calls ~Effect(), not ~EffectImpl(). Making ~Effect() virtual fixes this. This should make our Google3 tests sized-delete clean, unblocking those folks. Committed: https://skia.googlesource.com/skia/+/71aca549914c868d50cd51ddff3fb7a5bfe250cd ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/71aca549914c868d50cd51ddff3fb7a5bfe250cd |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
