Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2723083004:
Remove a scoped_refptr<>::swap overload (Closed)
|
Created:
3 years, 9 months ago by tzik Modified:
3 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, vmpstr+watch_chromium.org, posciak+watch_chromium.org, gavinp+memory_chromium.org, net-reviews_chromium.org, feature-media-reviews_chromium.org, gavinp Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove a scoped_refptr<>::swap overload
A scoped_refptr<>::swap overload is used to insert or remove the internal
pointer without manipulating the ref count. That functionality should not
be exposed, or at least, should be exposed separately with better name.
This CL removes the swap() overload, and rewrites callers without swap().
Review-Url: https://codereview.chromium.org/2723083004
Cr-Original-Commit-Position: refs/heads/master@{#455407}
Committed: https://chromium.googlesource.com/chromium/src/+/511c810d540422faf221bd82a4532313cabcc4c4
Review-Url: https://codereview.chromium.org/2723083004
Cr-Commit-Position: refs/heads/master@{#456685}
Committed: https://chromium.googlesource.com/chromium/src/+/93906711428eb47c0b9feeb0916fc91b1218afa3
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : fix #
Total comments: 5
Patch Set 4 : rebase on crrev.com/2733693002 #Patch Set 5 : avoid to use std::swap #
Messages
Total messages: 66 (40 generated)
The CQ bit was checked by tzik@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...
Description was changed from ========== Remove a scoped_refptr<>::swap overload ========== to ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrite callers without swap(). ==========
tzik@chromium.org changed reviewers: + dcheng@chromium.org
tzik@chromium.org changed reviewers: - dcheng@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tzik@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...
tzik@chromium.org changed reviewers: + dcheng@chromium.org
PTAL
Description was changed from ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrite callers without swap(). ========== to ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrites callers without swap(). ==========
https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... File net/disk_cache/blockfile/backend_impl.cc (right): https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... net/disk_cache/blockfile/backend_impl.cc:544: parent_entry->Release(); Is there any chance we can fix the //net code to just use scoped_refptr where appropriate rather than relying on manual refcount adjustments? It seems quite hard to verify that everything actually matches up...
LGTM But maybe let's file a followup bug and assign to //net team... the manual refcount maintenance seems like it's asking for trouble.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
tzik@chromium.org changed reviewers: + jkarlin@chromium.org
tzik@chromium.org changed reviewers: + mmenke@chromium.org, xhwang@chromium.org
Adding //net and //media owners. PTAL to: jkarlin: //net/disk_cache, mmenke: //net as an owner, xhwang: //media https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... File net/disk_cache/blockfile/backend_impl.cc (right): https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... net/disk_cache/blockfile/backend_impl.cc:544: parent_entry->Release(); On 2017/03/02 18:40:19, dcheng wrote: > Is there any chance we can fix the //net code to just use scoped_refptr where > appropriate rather than relying on manual refcount adjustments? It seems quite > hard to verify that everything actually matches up... Agree. It's very hard to read even for writing small CL like this.
Description was changed from ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrites callers without swap(). ========== to ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrites callers without swap(). BUG=TBD ==========
On 2017/03/02 19:26:29, tzik wrote: > Adding //net and //media owners. > PTAL to: > jkarlin: //net/disk_cache, > mmenke: //net as an owner, > xhwang: //media > > https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... > File net/disk_cache/blockfile/backend_impl.cc (right): > > https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... > net/disk_cache/blockfile/backend_impl.cc:544: parent_entry->Release(); > On 2017/03/02 18:40:19, dcheng wrote: > > Is there any chance we can fix the //net code to just use scoped_refptr where > > appropriate rather than relying on manual refcount adjustments? It seems quite > > hard to verify that everything actually matches up... > > Agree. It's very hard to read even for writing small CL like this. jkarlin: BTW, I heard the blockfile cache backend has been deprecated in favor to use the simple cache backend. Will the migration happen soon?
This makes the code even less readable. I don't think I like this approach. Better to switch to scoped_refptr in net before removing swap. WDYT? https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... File net/disk_cache/blockfile/backend_impl.cc (right): https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... net/disk_cache/blockfile/backend_impl.cc:544: parent_entry->Release(); On 2017/03/02 19:26:28, tzik wrote: > On 2017/03/02 18:40:19, dcheng wrote: > > Is there any chance we can fix the //net code to just use scoped_refptr where > > appropriate rather than relying on manual refcount adjustments? It seems quite > > hard to verify that everything actually matches up... > > Agree. It's very hard to read even for writing small CL like this. +1
On 2017/03/03 06:29:30, tzik wrote: > On 2017/03/02 19:26:29, tzik wrote: > > Adding //net and //media owners. > > PTAL to: > > jkarlin: //net/disk_cache, > > mmenke: //net as an owner, > > xhwang: //media > > > > > https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... > > File net/disk_cache/blockfile/backend_impl.cc (right): > > > > > https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... > > net/disk_cache/blockfile/backend_impl.cc:544: parent_entry->Release(); > > On 2017/03/02 18:40:19, dcheng wrote: > > > Is there any chance we can fix the //net code to just use scoped_refptr > where > > > appropriate rather than relying on manual refcount adjustments? It seems > quite > > > hard to verify that everything actually matches up... > > > > Agree. It's very hard to read even for writing small CL like this. > > jkarlin: > BTW, I heard the blockfile cache backend has been deprecated in favor to use the > simple cache backend. Will the migration happen soon? We'd like to deprecate it, but the SimpleCache replacement has hit snags (regressing) on Windows and OSX so we're not sure yet if it's going to happen.
LGTM % comment https://codereview.chromium.org/2723083004/diff/40001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2723083004/diff/40001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:128: opaque->AddRef(); This is hard to read (not saying it's worse than before :) )... Is there any plan to improve the API on this kind of use case?
https://codereview.chromium.org/2723083004/diff/40001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2723083004/diff/40001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:128: opaque->AddRef(); On 2017/03/03 18:36:08, xhwang_slow wrote: > This is hard to read (not saying it's worse than before :) )... > > Is there any plan to improve the API on this kind of use case? Eh... this seems Good Enough (tm) for the use case where we have to leak a ref to C. I'm not really sure what else we could use here: I guess we could have LeakRef() on scoped_refptr, but if we don't need it, it might be best to avoid.
On 2017/03/03 15:30:32, jkarlin wrote: > This makes the code even less readable. I don't think I like this approach. > Better to switch to scoped_refptr in net before removing swap. WDYT? Hmm, the removal seems not a major surgery...? OK, let's fix it first. > > https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... > File net/disk_cache/blockfile/backend_impl.cc (right): > > https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... > net/disk_cache/blockfile/backend_impl.cc:544: parent_entry->Release(); > On 2017/03/02 19:26:28, tzik wrote: > > On 2017/03/02 18:40:19, dcheng wrote: > > > Is there any chance we can fix the //net code to just use scoped_refptr > where > > > appropriate rather than relying on manual refcount adjustments? It seems > quite > > > hard to verify that everything actually matches up... > > > > Agree. It's very hard to read even for writing small CL like this. > > +1
Description was changed from ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrites callers without swap(). BUG=TBD ========== to ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrites callers without swap(). ==========
The CQ bit was checked by tzik@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.
On 2017/03/04 16:16:06, tzik wrote: > On 2017/03/03 15:30:32, jkarlin wrote: > > This makes the code even less readable. I don't think I like this approach. > > Better to switch to scoped_refptr in net before removing swap. WDYT? > > Hmm, the removal seems not a major surgery...? OK, let's fix it first. > > > > > > https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... > > File net/disk_cache/blockfile/backend_impl.cc (right): > > > > > https://codereview.chromium.org/2723083004/diff/40001/net/disk_cache/blockfil... > > net/disk_cache/blockfile/backend_impl.cc:544: parent_entry->Release(); > > On 2017/03/02 19:26:28, tzik wrote: > > > On 2017/03/02 18:40:19, dcheng wrote: > > > > Is there any chance we can fix the //net code to just use scoped_refptr > > where > > > > appropriate rather than relying on manual refcount adjustments? It seems > > quite > > > > hard to verify that everything actually matches up... > > > > > > Agree. It's very hard to read even for writing small CL like this. > > > > +1 This doesn't look to touch any more, removing myself as a reviewer (Feel free to add me back, if that changes).
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2723083004/#ps60001 (title: "rebase on crrev.com/2733693002")
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tzik@chromium.org
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488954727531140,
"parent_rev": "2acea43a7272838a75e4c6b7e5473dd6f9c4aa79", "commit_rev":
"511c810d540422faf221bd82a4532313cabcc4c4"}
Message was sent while issue was closed.
Description was changed from ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrites callers without swap(). ========== to ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrites callers without swap(). Review-Url: https://codereview.chromium.org/2723083004 Cr-Commit-Position: refs/heads/master@{#455407} Committed: https://chromium.googlesource.com/chromium/src/+/511c810d540422faf221bd82a453... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/511c810d540422faf221bd82a453...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2732063005/ by estevenson@chromium.org. The reason for reverting is: Broke https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde....
Message was sent while issue was closed.
On 2017/03/08 15:24:06, estevenson wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/2732063005/ by mailto:estevenson@chromium.org. > > The reason for reverting is: Broke > https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde.... Looks like a compiler bug to me... A minimized repro case is so fragile. E.g. if we swap #include <cassert> and #include <algorithm>, the bot will no longer fail.
Message was sent while issue was closed.
Description was changed from ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrites callers without swap(). Review-Url: https://codereview.chromium.org/2723083004 Cr-Commit-Position: refs/heads/master@{#455407} Committed: https://chromium.googlesource.com/chromium/src/+/511c810d540422faf221bd82a453... ========== to ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrites callers without swap(). Review-Url: https://codereview.chromium.org/2723083004 Cr-Commit-Position: refs/heads/master@{#455407} Committed: https://chromium.googlesource.com/chromium/src/+/511c810d540422faf221bd82a453... ==========
tzik@chromium.org changed reviewers: + battre@chromium.org
On 2017/03/08 20:15:20, tzik wrote: > On 2017/03/08 15:24:06, estevenson wrote: > > A revert of this CL (patchset #4 id:60001) has been created in > > https://codereview.chromium.org/2732063005/ by mailto:estevenson@chromium.org. > > > > The reason for reverting is: Broke > > > https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde.... > > Looks like a compiler bug to me... A minimized repro case is so fragile. E.g. if > we swap #include <cassert> and #include <algorithm>, the bot will no longer > fail. I think a less ad-hoc solution is rolling flatbuffer to newer version to take below, and defining FLATBUFFERS_CPP98_STL to avoid the problematic usage of std::unique_ptr and std::function. https://github.com/google/flatbuffers/commit/7c7c571bbec3e698b17a191cb5616f94... battre: Is this feasible to you?
battre@chromium.org changed reviewers: + pkalinnikov@chromium.org
On 2017/03/08 20:33:22, tzik wrote: > On 2017/03/08 20:15:20, tzik wrote: > > On 2017/03/08 15:24:06, estevenson wrote: > > > A revert of this CL (patchset #4 id:60001) has been created in > > > https://codereview.chromium.org/2732063005/ by > mailto:estevenson@chromium.org. > > > > > > The reason for reverting is: Broke > > > > > > https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde.... > > > > Looks like a compiler bug to me... A minimized repro case is so fragile. E.g. > if > > we swap #include <cassert> and #include <algorithm>, the bot will no longer > > fail. > > I think a less ad-hoc solution is rolling flatbuffer to newer version to take > below, and defining FLATBUFFERS_CPP98_STL to avoid the problematic usage of > std::unique_ptr and std::function. > https://github.com/google/flatbuffers/commit/7c7c571bbec3e698b17a191cb5616f94... > > battre: Is this feasible to you? Pavel, do you want to look at this? Otherwise, I will take a look on Friday.
On 2017/03/08 21:38:42, battre wrote: > On 2017/03/08 20:33:22, tzik wrote: > > On 2017/03/08 20:15:20, tzik wrote: > > > On 2017/03/08 15:24:06, estevenson wrote: > > > > A revert of this CL (patchset #4 id:60001) has been created in > > > > https://codereview.chromium.org/2732063005/ by > > mailto:estevenson@chromium.org. > > > > > > > > The reason for reverting is: Broke > > > > > > > > > > https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde.... > > > > > > Looks like a compiler bug to me... A minimized repro case is so fragile. > E.g. > > if > > > we swap #include <cassert> and #include <algorithm>, the bot will no longer > > > fail. > > > > I think a less ad-hoc solution is rolling flatbuffer to newer version to take > > below, and defining FLATBUFFERS_CPP98_STL to avoid the problematic usage of > > std::unique_ptr and std::function. > > > https://github.com/google/flatbuffers/commit/7c7c571bbec3e698b17a191cb5616f94... > > > > battre: Is this feasible to you? > > Pavel, do you want to look at this? Otherwise, I will take a look on Friday. This sounds good to me. Do you expect FLATBUFFERS_CPP98_STL automatically defined given the above commit, or we should define it manually? Also, seems like FlatBuffers is not checked out from a release label (https://github.com/google/flatbuffers/releases). Would be also nice to fix this. Any release starting from 1.4.0 includes the mentioned commit. I would try 1.5.0, since 1.6.0 is quite recent and includes extra things like FlexBuffers that we don't use yet.
I am working on a CL to update FlatBuffers version: https://codereview.chromium.org/2740103002/
The CQ bit was checked by tzik@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...
Removed #include <algorithm> as a workaround of the failure. This CL should work even without the flatbuffer update. # Though its update is still needed, since it's still fragile.
The CQ bit was unchecked by tzik@chromium.org
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2723083004/#ps80001 (title: "avoid to use std::swap")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489494358705840,
"parent_rev": "7bbaaf780f18e55c5f7055933f557b49cfb8dd90", "commit_rev":
"93906711428eb47c0b9feeb0916fc91b1218afa3"}
Message was sent while issue was closed.
Description was changed from ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrites callers without swap(). Review-Url: https://codereview.chromium.org/2723083004 Cr-Commit-Position: refs/heads/master@{#455407} Committed: https://chromium.googlesource.com/chromium/src/+/511c810d540422faf221bd82a453... ========== to ========== Remove a scoped_refptr<>::swap overload A scoped_refptr<>::swap overload is used to insert or remove the internal pointer without manipulating the ref count. That functionality should not be exposed, or at least, should be exposed separately with better name. This CL removes the swap() overload, and rewrites callers without swap(). Review-Url: https://codereview.chromium.org/2723083004 Cr-Original-Commit-Position: refs/heads/master@{#455407} Committed: https://chromium.googlesource.com/chromium/src/+/511c810d540422faf221bd82a453... Review-Url: https://codereview.chromium.org/2723083004 Cr-Commit-Position: refs/heads/master@{#456685} Committed: https://chromium.googlesource.com/chromium/src/+/93906711428eb47c0b9feeb0916f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/93906711428eb47c0b9feeb0916f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
