Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1770723002:
Add variants of the setters on SkPaint which take a sk_sp<effect> (Closed)
DescriptionAdd variants of the setters on SkPaint which take a sk_sp<effect>. At the same time, change the internal storage to be sk_sp<effect>.
Follow-on CL might try to use = default for the constructors and assignment operators.
This reverts commit 992854d62e179a589aa7366e443246e3672c3248.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1770723002
Committed: https://skia.googlesource.com/skia/+/a5ab9ec295b2e6dca166775a98db67a9a8c18c37
Patch Set 1 #Patch Set 2 : simplify/unify macros in constructors for paint #
Total comments: 2
Patch Set 3 : #
Messages
Total messages: 27 (11 generated)
Description was changed from ========== add setter on SkPaint that takes sk_sp (patchset #1 id:1 of https://codereview.chromium.org/1769803002/ )" This reverts commit 992854d62e179a589aa7366e443246e3672c3248. BUG=skia: ========== to ========== add setter on SkPaint that takes sk_sp (patchset #1 id:1 of https://codereview.chromium.org/1769803002/ )" This reverts commit 992854d62e179a589aa7366e443246e3672c3248. 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/1770723002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1770723002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@google.com changed reviewers: + bungeman@google.com, fmalita@chromium.org, halcanary@google.com, mtklein@google.com
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/1770723002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1770723002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/03/06 20:34:49, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Do the defaults work?
https://codereview.chromium.org/1770723002/diff/20001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/1770723002/diff/20001/include/core/SkPaint.h#... include/core/SkPaint.h:53: SkPaint& operator=(SkPaint&&); SkPaint& operator=(const SkPaint&) = default; SkPaint& operator=(SkPaint&&) = default; SkPaint(const SkPaint&) = default; SkPaint(SkPaint&&) = default;
I was wondering the same thing...
When can we stop supporting msvs2013? https://codereview.chromium.org/1770723002/diff/20001/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/1770723002/diff/20001/include/core/SkPaint.h#... include/core/SkPaint.h:53: SkPaint& operator=(SkPaint&&); On 2016/03/06 21:21:19, Hal Canary wrote: > SkPaint& operator=(const SkPaint&) = default; > SkPaint& operator=(SkPaint&&) = default; > SkPaint(const SkPaint&) = default; > SkPaint(SkPaint&&) = default; We have to be really careful about this. vc++ in msvs2013 doesn't support =default for the move constructor or move assignment operator and won't automatically generate them either. See https://msdn.microsoft.com/en-us/library/hh409293(v=vs.120).aspx#alert_note . On the other hand, doing this for the copies is probably the right thing to do.
If there's any doubt, perhaps we can land this as is, as it will free up other CLs/experiments since it adds the new setter style, and then we can try out using = default... ?
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/1770723002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1770723002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/03/06 22:04:24, reed1 wrote: > If there's any doubt, perhaps we can land this as is, as it will free up other > CLs/experiments since it adds the new setter style, and then we can try out > using = default... ? I just tried locally (happen to have msvs2013 here) and '= default' works with the copy constructor and copy assignment operator, but using '= default' with the move constructor or move assignment operator results in the compiler reporting an error. I'm fine with this as-is though. lgtm The description / commit message could be a little clearer.
On 2016/03/06 22:36:55, bungeman1 wrote: > On 2016/03/06 22:04:24, reed1 wrote: > > If there's any doubt, perhaps we can land this as is, as it will free up other > > CLs/experiments since it adds the new setter style, and then we can try out > > using = default... ? > > I just tried locally (happen to have msvs2013 here) and '= default' works with > the copy constructor and copy assignment operator, but using '= default' with > the move constructor or move assignment operator results in the compiler > reporting an error. I'm fine with this as-is though. lgtm > > The description / commit message could be a little clearer. Thanks for answering my question.
Description was changed from ========== add setter on SkPaint that takes sk_sp (patchset #1 id:1 of https://codereview.chromium.org/1769803002/ )" This reverts commit 992854d62e179a589aa7366e443246e3672c3248. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add variants of the setters on SkPaint which take a sk_sp<effect>. At the same time, change the internal storage to be sk_sp<effect>. Follow-on CL might try to use = default for the constructors and assignment operators. This reverts commit 992854d62e179a589aa7366e443246e3672c3248. 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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1770723002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1770723002/40001
Message was sent while issue was closed.
Description was changed from ========== Add variants of the setters on SkPaint which take a sk_sp<effect>. At the same time, change the internal storage to be sk_sp<effect>. Follow-on CL might try to use = default for the constructors and assignment operators. This reverts commit 992854d62e179a589aa7366e443246e3672c3248. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add variants of the setters on SkPaint which take a sk_sp<effect>. At the same time, change the internal storage to be sk_sp<effect>. Follow-on CL might try to use = default for the constructors and assignment operators. This reverts commit 992854d62e179a589aa7366e443246e3672c3248. 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/+/a5ab9ec295b2e6dca166775a98db67a9a8c18c37 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/a5ab9ec295b2e6dca166775a98db67a9a8c18c37 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
