Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1769803002:
add setter on SkPaint that takes sk_sp (Closed)
|
Created:
4 years, 9 months ago by reed1 Modified:
4 years, 9 months ago Reviewers:
bungeman-skia CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
Descriptionadd setter on SkPaint that takes sk_sp
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1769803002
TBR=
Committed: https://skia.googlesource.com/skia/+/a917eba6ea8a6936f2c9271e487b14d14b99c98e
Patch Set 1 #
Total comments: 5
Messages
Total messages: 15 (7 generated)
Description was changed from ========== add setter on SkPaint that takes sk_sp BUG=skia: ========== to ========== add setter on SkPaint that takes sk_sp 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/1769803002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769803002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== add setter on SkPaint that takes sk_sp BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== add setter on SkPaint that takes sk_sp BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... TBR= ==========
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/1769803002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769803002/1
Message was sent while issue was closed.
Description was changed from ========== add setter on SkPaint that takes sk_sp BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... TBR= ========== to ========== add setter on SkPaint that takes sk_sp BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... TBR= Committed: https://skia.googlesource.com/skia/+/a917eba6ea8a6936f2c9271e487b14d14b99c98e ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/a917eba6ea8a6936f2c9271e487b14d14b99c98e
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1771613002/ by reed@google.com. The reason for reverting is: investigate leak.
Message was sent while issue was closed.
bungeman@google.com changed reviewers: + bungeman@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1769803002/diff/1/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/1769803002/diff/1/src/core/SkPaint.cpp#newcode66 src/core/SkPaint.cpp:66: #define COPY_CTR(field) field(src.field) Note that for simplicity and uniformity, COPY_CTR can just be renamed COPY, the existing COPY below be removed, and just do everything in the member initializer list. Then you don't need two names and everything looks the same. Interestingly, it looks like you already did basically this in the operator=(const&) below. https://codereview.chromium.org/1769803002/diff/1/src/core/SkPaint.cpp#newcode92 src/core/SkPaint.cpp:92: #define REF_MOVE(field) field = src.field; src.field = nullptr REF_MOVE needs to be removed and all the REF_MOVE below just turn into MOVE. Indeed, this should end up like the copy constructor above, but with MOVE instead of COPY, with everything done in the member initializer list. This is probably the source of the leak, as the assignment here is actually calling the copy assignment operator. It looks like you already did this with move assignment operator.
Message was sent while issue was closed.
https://codereview.chromium.org/1769803002/diff/1/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/1769803002/diff/1/src/core/SkPaint.cpp#newcode92 src/core/SkPaint.cpp:92: #define REF_MOVE(field) field = src.field; src.field = nullptr On 2016/03/06 04:50:05, bungeman1 wrote: > REF_MOVE needs to be removed and all the REF_MOVE below just turn into MOVE. > Indeed, this should end up like the copy constructor above, but with MOVE > instead of COPY, with everything done in the member initializer list. This is > probably the source of the leak, as the assignment here is actually calling the > copy assignment operator. It looks like you already did this with move > assignment operator. I shouldn't write these things so late at night. Of course this should not leak. However, this is calling the copy assignment operator so it's not a very efficient move.
Message was sent while issue was closed.
moved to https://codereview.chromium.org/1770723002# https://codereview.chromium.org/1769803002/diff/1/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/1769803002/diff/1/src/core/SkPaint.cpp#newcode66 src/core/SkPaint.cpp:66: #define COPY_CTR(field) field(src.field) On 2016/03/06 04:50:05, bungeman1 wrote: > Note that for simplicity and uniformity, COPY_CTR can just be renamed COPY, the > existing COPY below be removed, and just do everything in the member initializer > list. Then you don't need two names and everything looks the same. > Interestingly, it looks like you already did basically this in the > operator=(const&) below. Agreed about unity. I think CTR is clearer (to me) than COPY, but I'll go with COPY. https://codereview.chromium.org/1769803002/diff/1/src/core/SkPaint.cpp#newcode92 src/core/SkPaint.cpp:92: #define REF_MOVE(field) field = src.field; src.field = nullptr On 2016/03/06 14:03:40, bungeman1 wrote: > On 2016/03/06 04:50:05, bungeman1 wrote: > > REF_MOVE needs to be removed and all the REF_MOVE below just turn into MOVE. > > Indeed, this should end up like the copy constructor above, but with MOVE > > instead of COPY, with everything done in the member initializer list. This is > > probably the source of the leak, as the assignment here is actually calling > the > > copy assignment operator. It looks like you already did this with move > > assignment operator. > > I shouldn't write these things so late at night. Of course this should not leak. > However, this is calling the copy assignment operator so it's not a very > efficient move. Doh! I inverted the intended use. Will move all of them. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
