Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1752683002:
isolate sk_sp from larger cl (Closed)
Descriptionisolate sk_sp from larger cl
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1752683002
Committed: https://skia.googlesource.com/skia/+/bb7b043b2db64197d2f6a1edaf3562a50c77afb1
Patch Set 1 #
Total comments: 5
Patch Set 2 : add some dox #Patch Set 3 : add default arg to reset #
Total comments: 4
Patch Set 4 : remove default arg to bare-ptr constructor now that we have an explicit default constructor #
Total comments: 5
Patch Set 5 : spelling mistakes #
Messages
Total messages: 31 (13 generated)
Description was changed from ========== isolate sk_sp from larger cl BUG=skia: ========== to ========== isolate sk_sp from larger cl 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 reed@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/1752683002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752683002/1
reed@google.com changed reviewers: + bungeman@google.com, fmalita@chromium.org, mtklein@google.com
isolated from larger maskfilter CL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
fmalita@google.com changed reviewers: + fmalita@google.com
https://codereview.chromium.org/1752683002/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1752683002/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:233: public: Default ctor? https://codereview.chromium.org/1752683002/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:237: explicit sk_sp(T* obj = nullptr) : fPtr(obj) {} The adoption is not self documenting, and it's bound to be confusing in Chromium/Blink land. How about an explicit Adopt() instead?
https://codereview.chromium.org/1752683002/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1752683002/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:237: explicit sk_sp(T* obj = nullptr) : fPtr(obj) {} On 2016/03/01 05:23:25, fmalita_google_do_not_use wrote: > The adoption is not self documenting, and it's bound to be confusing in > Chromium/Blink land. How about an explicit Adopt() instead? OTOH this makes sk_sp a drop-in replacment for SkAutoTUnref, which I think is desirable. Don't let me slow this down, we can tweak later. LGTM.
https://codereview.chromium.org/1752683002/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1752683002/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:233: public: On 2016/03/01 05:23:25, fmalita_google_do_not_use wrote: > Default ctor? (T* obj = nullptr) serves as the default. https://codereview.chromium.org/1752683002/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:237: explicit sk_sp(T* obj = nullptr) : fPtr(obj) {} On 2016/03/01 12:53:49, f(malita) wrote: > On 2016/03/01 05:23:25, fmalita_google_do_not_use wrote: > > The adoption is not self documenting, and it's bound to be confusing in > > Chromium/Blink land. How about an explicit Adopt() instead? > > OTOH this makes sk_sp a drop-in replacment for SkAutoTUnref, which I think is > desirable. Don't let me slow this down, we can tweak later. LGTM. Let's plan for very few bare pointers escape into Chromium/Blink land. The usual interaction should be with the other constructors and operators. But I think it can't hurt to move this up a bit on its own and write something like // Adopts ownership of the reference of an existing bare T*, mostly for transitional compatibility.
Added some dox. Note that I've removed the auto-conversion to T* (since I no longer inherit from anything), so hopefully this will make it more strict/clean, and perhaps more acceptable to other clients. I propose landing this w/o further changes to skia, though no doubt those will follow. I want to enable our template experts to fiddle to their hearts' content now that we have a base behavior established.
The CQ bit was checked by reed@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/1752683002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752683002/20001
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 reed@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/1752683002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752683002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by reed@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/1752683002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752683002/60001
https://codereview.chromium.org/1752683002/diff/40001/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1752683002/diff/40001/include/core/SkRefCnt.h... include/core/SkRefCnt.h:237: * may have its ref/unref be thread-safe, but that is not assumed/inposed by sk_sp. imposed https://codereview.chromium.org/1752683002/diff/40001/include/core/SkRefCnt.h... include/core/SkRefCnt.h:246: * create sk_sp both have a reference to it. -> created
https://codereview.chromium.org/1752683002/diff/40001/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1752683002/diff/40001/include/core/SkRefCnt.h... include/core/SkRefCnt.h:237: * may have its ref/unref be thread-safe, but that is not assumed/inposed by sk_sp. On 2016/03/01 15:10:38, mtklein wrote: > imposed Done. https://codereview.chromium.org/1752683002/diff/40001/include/core/SkRefCnt.h... include/core/SkRefCnt.h:246: * create sk_sp both have a reference to it. On 2016/03/01 15:10:38, mtklein wrote: > -> created Done.
https://codereview.chromium.org/1752683002/diff/60001/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1752683002/diff/60001/include/core/SkRefCnt.h... include/core/SkRefCnt.h:248: sk_sp(const sk_sp<T>& that) : fPtr(SkSafeRef(that.get())) {} Gonna mark some functions that, in my opinion, have obvious unsurprising semantics and implementations and which might read more clearly without comments. This one. https://codereview.chromium.org/1752683002/diff/60001/include/core/SkRefCnt.h... include/core/SkRefCnt.h:255: sk_sp(sk_sp<T>&& that) : fPtr(that.release()) {} This one. https://codereview.chromium.org/1752683002/diff/60001/include/core/SkRefCnt.h... include/core/SkRefCnt.h:266: ~sk_sp() { This one. https://codereview.chromium.org/1752683002/diff/60001/include/core/SkRefCnt.h... include/core/SkRefCnt.h:277: sk_sp<T>& operator=(const sk_sp<T>& that) { This one. https://codereview.chromium.org/1752683002/diff/60001/include/core/SkRefCnt.h... include/core/SkRefCnt.h:287: sk_sp<T>& operator=(sk_sp<T>&& that) { This one.
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org, mtklein@google.com Link to the patchset: https://codereview.chromium.org/1752683002/#ps80001 (title: "spelling mistakes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1752683002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752683002/80001
Message was sent while issue was closed.
Description was changed from ========== isolate sk_sp from larger cl BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== isolate sk_sp from larger cl BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/bb7b043b2db64197d2f6a1edaf3562a50c77afb1 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/bb7b043b2db64197d2f6a1edaf3562a50c77afb1 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
