Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2232913003:
Add a SkTLazy copy assignment operator (Closed)
DescriptionAdd a SkTLazy copy assignment operator
Also scrub for NULL, etc.
R=mtklein@google.com,reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2232913003
Committed: https://skia.googlesource.com/skia/+/0cbe77c383a1c829341b27df1a9219bc33524440
Patch Set 1 #
Total comments: 4
Patch Set 2 : review #Messages
Total messages: 25 (14 generated)
Description was changed from ========== Add a SkTLazy copy assignment operator Also scrub for NULL, etc. R=mtklein@google.com,reed@google.com ========== to ========== Add a SkTLazy copy assignment operator Also scrub for NULL, etc. R=mtklein@google.com,reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2232913003 ==========
No SkTLazy assignment creates some friction - any reason not to add it?
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yeah, lgtm with a couple suggestions. I can't think of any reason why a copy constructor would be safe but operator= not. https://codereview.chromium.org/2232913003/diff/1/include/core/SkTLazy.h File include/core/SkTLazy.h (right): https://codereview.chromium.org/2232913003/diff/1/include/core/SkTLazy.h#newc... include/core/SkTLazy.h:27: SkTLazy(const SkTLazy<T>& src) It can be reassuring to see the copy constructor implemented in terms of operator= when it doesn't change meaning or hurt performance. https://codereview.chromium.org/2232913003/diff/1/include/core/SkTLazy.h#newc... include/core/SkTLazy.h:36: SkTLazy& operator=(const SkTLazy<T>& src) { I think there's no need for the <T> in these argument lists. It's fine I guess, but we seem inconsistent... take SkTLazy<T>, return SkTLazy...
https://codereview.chromium.org/2232913003/diff/1/include/core/SkTLazy.h File include/core/SkTLazy.h (right): https://codereview.chromium.org/2232913003/diff/1/include/core/SkTLazy.h#newc... include/core/SkTLazy.h:27: SkTLazy(const SkTLazy<T>& src) On 2016/08/10 18:59:46, mtklein wrote: > It can be reassuring to see the copy constructor implemented in terms of > operator= when it doesn't change meaning or hurt performance. Done. This does add a couple of redundant isValid() checks, but with everything inlined the compiler will hopefully optimize them away? https://codereview.chromium.org/2232913003/diff/1/include/core/SkTLazy.h#newc... include/core/SkTLazy.h:36: SkTLazy& operator=(const SkTLazy<T>& src) { On 2016/08/10 18:59:46, mtklein wrote: > I think there's no need for the <T> in these argument lists. It's fine I guess, > but we seem inconsistent... take SkTLazy<T>, return SkTLazy... Done.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 fmalita@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/2232913003/#ps20001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
On 2016/08/10 20:27:57, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED, > http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...) Requires public API blessing.
lgtm do we think we can move this into include/private ? (separate CL/question)
On 2016/08/10 20:33:19, reed1 wrote: > lgtm > > do we think we can move this into include/private ? (separate CL/question) Possibly. SkTLazy is safe AFAICT, but I see a couple of SkTCopyOnFirstWrite users in cc/ and skia/.
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> This does add a couple of redundant isValid() checks, but with everything > inlined the compiler will hopefully optimize them away? Yeah, that's what I'd expect. It of course never hurts to compile before and after and look at the disassembly diff.
Message was sent while issue was closed.
Description was changed from ========== Add a SkTLazy copy assignment operator Also scrub for NULL, etc. R=mtklein@google.com,reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2232913003 ========== to ========== Add a SkTLazy copy assignment operator Also scrub for NULL, etc. R=mtklein@google.com,reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2232913003 Committed: https://skia.googlesource.com/skia/+/0cbe77c383a1c829341b27df1a9219bc33524440 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/0cbe77c383a1c829341b27df1a9219bc33524440 |
