Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1773453002:
Add element_type, swap, operators, fix reset on sk_sp. (Closed)
|
Created:
4 years, 9 months ago by bungeman-skia Modified:
4 years, 9 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd element_type, swap, operators, fix reset on sk_sp.
The 'element_type' typedef is to play nice with std::pointer_traits.
The full complement of operators and swap to match unique_ptr so that
sk_sp can be properly compared to nullptr and used with standard
containers.
Update to 'reset' so that calling 'unref' is the last operation.
This also adds tests for these changes, and sets the fPtr to nullptr
in debug for easier bug finding.
Committed: https://skia.googlesource.com/skia/+/c901c11549acd19e7fc225276f78374ac1600496
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add additional reset test. #
Total comments: 2
Patch Set 3 : Clang! You were the chosen one! #
Messages
Total messages: 29 (16 generated)
Description was changed from ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. ========== to ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. ==========
The CQ bit was checked by bungeman@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/1773453002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773453002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bungeman@google.com changed reviewers: + halcanary@google.com, mtklein@google.com, reed@google.com
This adds most of the basic function that unique_ptr has that sk_sp doesn't have yet.
lgtm https://codereview.chromium.org/1773453002/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1773453002/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:354: swap(fPtr, that.fPtr); `std::swap(fPtr, that.fPtr);` is less verbose. You must have a good reason not to use it.
https://codereview.chromium.org/1773453002/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1773453002/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:354: swap(fPtr, that.fPtr); On 2016/03/07 16:00:25, Hal Canary wrote: > `std::swap(fPtr, that.fPtr);` is less verbose. You must have a good reason not > to use it. Yeah, this is the correct way to call swap so that ADL kicks in. That being said, it does seem unlikely that one would specialize swapping T*, but it doesn't hurt to just write it the standard way. Apparently I also didn't get around to writing a test for this swap. I also didn't write SkTSwap for Skia, but maybe we should just use std::swap at this point. http://en.cppreference.com/w/cpp/concept/Swappable
Description was changed from ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. ========== to ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. ==========
https://codereview.chromium.org/1773453002/diff/20001/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1773453002/diff/20001/include/core/SkRefCnt.h... include/core/SkRefCnt.h:332: void reset(T* ptr = nullptr) { At the very least, Chromium went through a lot of work to get scoped_ptr to do this, and doing so uncovered bugs (see https://bugs.chromium.org/p/chromium/issues/detail?id=176091). I believe it would be one less point of contention with Chromium to do things this way. https://codereview.chromium.org/1773453002/diff/20001/tests/RefCntTest.cpp File tests/RefCntTest.cpp (right): https://codereview.chromium.org/1773453002/diff/20001/tests/RefCntTest.cpp#ne... tests/RefCntTest.cpp:325: // https://crrev.com/0d4ef2583a6f19c3e61be04d36eb1a60b133832c This is basically the test that Chromium has for the reset issue Geven who was involved in the issue998 discussion, this may be the 'better' example of the issue.
The CQ bit was checked by bungeman@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/1773453002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773453002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
Description was changed from ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. ========== to ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. ==========
The CQ bit was checked by bungeman@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/1773453002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773453002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
api lgtm
The CQ bit was checked by bungeman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from halcanary@google.com Link to the patchset: https://codereview.chromium.org/1773453002/#ps40001 (title: "Clang! You were the chosen one!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773453002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773453002/40001
Message was sent while issue was closed.
Description was changed from ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. ========== to ========== Add element_type, swap, operators, fix reset on sk_sp. The 'element_type' typedef is to play nice with std::pointer_traits. The full complement of operators and swap to match unique_ptr so that sk_sp can be properly compared to nullptr and used with standard containers. Update to 'reset' so that calling 'unref' is the last operation. This also adds tests for these changes, and sets the fPtr to nullptr in debug for easier bug finding. Committed: https://skia.googlesource.com/skia/+/c901c11549acd19e7fc225276f78374ac1600496 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/c901c11549acd19e7fc225276f78374ac1600496 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
