Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1767983002:
Fix behavior of sk_sp::reset() and add unittest. (Closed)
DescriptionFix behavior of sk_sp::reset(T*) and add unittest.
Previously, sk_sp::reset(T* t) did not release its own reference
if its internal pointer was the same as 't'. This leaks a reference.
Now always release the current reference when non-nullptr.
Committed: https://skia.googlesource.com/skia/+/941da9d66171bd8efd2f6c5e25ff90c8c69885c1
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : use unique() correctly #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== fix behavior of sk_sp.reset() and add unittest BUG=skia: ========== to ========== fix behavior of sk_sp.reset() and add unittest BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
reed@google.com changed reviewers: + bungeman@google.com, 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/1767983002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767983002/1
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/1767983002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767983002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1767983002/diff/20001/tests/RefCntTest.cpp File tests/RefCntTest.cpp (right): https://codereview.chromium.org/1767983002/diff/20001/tests/RefCntTest.cpp#ne... tests/RefCntTest.cpp:298: REPORTER_ASSERT(r, 1 == rc->unique()); This reads a little funny with the unexpected thinking about the 1 converting to bool, this should either just be 'rc->unique()' (and below ('!rc->unique()' instead of '2 == rc->getRefCnt()) or it should be '1 == rc->getRefCnt()'.
Doh! Good point. I had used getRefCnt everywhere, but then discovered that it was debug-only. I'll convert to unique everywhere.
ptal https://codereview.chromium.org/1767983002/diff/20001/tests/RefCntTest.cpp File tests/RefCntTest.cpp (right): https://codereview.chromium.org/1767983002/diff/20001/tests/RefCntTest.cpp#ne... tests/RefCntTest.cpp:298: REPORTER_ASSERT(r, 1 == rc->unique()); On 2016/03/06 14:01:44, bungeman1 wrote: > This reads a little funny with the unexpected thinking about the 1 converting to > bool, this should either just be 'rc->unique()' (and below ('!rc->unique()' > instead of '2 == rc->getRefCnt()) or it should be '1 == rc->getRefCnt()'. Done.
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/1767983002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767983002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Description was changed from ========== fix behavior of sk_sp.reset() and add unittest BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix behavior of sk_sp::reset(T*) and add unittest. Previously, sk_sp::reset(T* t) did not release its own reference if its internal pointer was the same as 't'. This leaks a reference. Now always release the current reference when non-nullptr. ==========
Description was changed from ========== Fix behavior of sk_sp::reset(T*) and add unittest. Previously, sk_sp::reset(T* t) did not release its own reference if its internal pointer was the same as 't'. This leaks a reference. Now always release the current reference when non-nullptr. ========== to ========== Fix behavior of sk_sp::reset(T*) and add unittest. Previously, sk_sp::reset(T* t) did not release its own reference if its internal pointer was the same as 't'. This leaks a reference. Now always release the current reference when non-nullptr. ==========
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767983002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767983002/40001
Message was sent while issue was closed.
Description was changed from ========== Fix behavior of sk_sp::reset(T*) and add unittest. Previously, sk_sp::reset(T* t) did not release its own reference if its internal pointer was the same as 't'. This leaks a reference. Now always release the current reference when non-nullptr. ========== to ========== Fix behavior of sk_sp::reset(T*) and add unittest. Previously, sk_sp::reset(T* t) did not release its own reference if its internal pointer was the same as 't'. This leaks a reference. Now always release the current reference when non-nullptr. Committed: https://skia.googlesource.com/skia/+/941da9d66171bd8efd2f6c5e25ff90c8c69885c1 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/941da9d66171bd8efd2f6c5e25ff90c8c69885c1 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
