Remove std::move(raw pointer) |
|||
Issue descriptionCalling std::move on a raw pointer seems pointless, because it ends up doing the same as just copying the raw pointer would do. Furthermore, it can indicate a programming error, where the original intent was to move, e.g., a unique_ptr, but the programmer forgot to convert the raw pointer to a unique_ptr. Therefore, occurrences of std::move called on raw pointers will be looked for in Chromium code with a clang tool, and fixed. This is a follow-up to https://crbug.com/581865#c66.
,
Jun 4 2017
Results from running on Linux: ../../components/browser_sync/profile_sync_service.cc:10101 ../../components/sync_sessions/sessions_sync_manager.cc:5987 fixed in https://codereview.chromium.org/2923453002/ ../../content/browser/indexed_db/indexed_db_transaction.cc:3865 fixed in https://codereview.chromium.org/2923463002/ ../../gpu/ipc/service/gpu_channel.cc:26467 fixed in https://codereview.chromium.org/2920133002/ ../../services/preferences/tracked/pref_hash_store_impl.cc:3329 ../../services/preferences/tracked/pref_hash_store_impl.cc:4518 ../../services/ui/public/cpp/bitmap/child_shared_bitmap_manager.cc:5009 ../../services/video_capture/test/mock_device_factory.cc:2266 fixed in https://codereview.chromium.org/2919253002/ ../../third_party/flatbuffers/src/include/flatbuffers/flatbuffers.h:8260 TBD ../../third_party/skia/include/core/SkCanvas.h:14756 TBD The following instances are in templates, where the templated type may or may not be a raw pointer, hence the move cannot be dropped. ../../third_party/webrtc/api/proxy.h:16937 ../../third_party/webrtc/api/proxy.h:19681 ../../third_party/webrtc/api/proxy.h:20241 ../../build/linux/debian_jessie_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/move.h:4428 ../../build/linux/debian_jessie_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_algobase.h:11759 ../../build/linux/debian_jessie_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_algobase.h:18499 ../../build/linux/debian_jessie_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_iterator.h:31553 There were also about 120 errors during processing (attached), which I might need to dig into. There were also about 12k skipped files (attached), which are not compiled on Linux. I might need to run the tool on the appropriate platform to check those.
,
Jun 4 2017
../../third_party/flatbuffers/src/include/flatbuffers/flatbuffers.h:8260 is now https://github.com/google/flatbuffers/pull/4339
,
Jun 4 2017
../../third_party/skia/include/core/SkCanvas.h:14756 is now https://skia-review.googlesource.com/c/18540/
,
Jun 4 2017
The 120 errors were due to an issue explained in https://codereview.chromium.org/2919243002#msg8 Out of those, there were 5 more cases discovered: ../../chrome/browser/ui/views/payments/test_chrome_payment_request_delegate.cc:955 fixed in https://codereview.chromium.org/2919263002/ ../../net/quic/chromium/quic_chromium_client_session.cc:11472 ../../net/url_request/url_request_unittest.cc:253222 fixed in https://codereview.chromium.org/2922883002/ ../../third_party/WebKit/Source/modules/sensor/Sensor.cpp:9395 ../../third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:15257 fixed in https://codereview.chromium.org/2919273002/ That's all for the moment, I might run the tool on other platforms later, once I have access to those.
,
Jun 5 2017
The following revision refers to this bug: https://skia.googlesource.com/skia/+/d0e0a8ff416abf9b4736ddac8f6faeb11023ab80 commit d0e0a8ff416abf9b4736ddac8f6faeb11023ab80 Author: Vaclav Brozek <vabr@chromium.org> Date: Mon Jun 05 01:17:51 2017 Don't move raw pointers in SkCanvas Moving raw pointers does the same job as copying, but is more verbose and also more confusing: e.g., is the supposed pointer meant to be a smart one? This instance was flagged by the tool from https://codereview.chromium.org/2919243002/. BUG= chromium:729393 Change-Id: I4c89e9d80fab9f6d14ab7db53e8b9b6e7cf966dc Reviewed-on: https://skia-review.googlesource.com/18540 Reviewed-by: Florin Malita <fmalita@chromium.org> Reviewed-by: Mike Reed <reed@google.com> Commit-Queue: Mike Reed <reed@google.com> [modify] https://crrev.com/d0e0a8ff416abf9b4736ddac8f6faeb11023ab80/include/core/SkCanvas.h
,
Jun 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e5cae49a7f640721bf81c4c68ff9b4dc8b24efa commit 2e5cae49a7f640721bf81c4c68ff9b4dc8b24efa Author: skia-deps-roller@chromium.org <skia-deps-roller@chromium.org> Date: Mon Jun 05 04:35:59 2017 Roll src/third_party/skia/ f4e498601..d0e0a8ff4 (3 commits) https://skia.googlesource.com/skia.git/+log/f4e498601755..d0e0a8ff416a $ git log f4e498601..d0e0a8ff4 --date=short --no-merges --format='%ad %ae %s' 2017-06-04 vabr Don't move raw pointers in SkCanvas 2017-06-04 mtklein don't try lowp in msan builds 2017-06-02 mtklein start on SkJumper lowp mode Created with: roll-dep src/third_party/skia BUG= 729393 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, see: http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel TBR=allanmac@chromium.org Change-Id: Id0399fc98770470c1e86b39a544aa4461211a3e7 Reviewed-on: https://chromium-review.googlesource.com/523642 Reviewed-by: Skia Deps Roller <skia-deps-roller@chromium.org> Commit-Queue: Skia Deps Roller <skia-deps-roller@chromium.org> Cr-Commit-Position: refs/heads/master@{#476939} [modify] https://crrev.com/2e5cae49a7f640721bf81c4c68ff9b4dc8b24efa/DEPS
,
Jun 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0afe568a073a2394d826c084f1878fd0ef19bd49 commit 0afe568a073a2394d826c084f1878fd0ef19bd49 Author: vabr <vabr@chromium.org> Date: Mon Jun 05 10:31:21 2017 Don't move raw pointers in third_party/WebKit Moving raw pointers does the same job as copying, but is more verbose and also more confusing: e.g., is the supposed pointer meant to be a smart one? This instance was flagged by the tool from https://codereview.chromium.org/2919243002/. BUG= 729393 Review-Url: https://codereview.chromium.org/2919273002 Cr-Commit-Position: refs/heads/master@{#476967} [modify] https://crrev.com/0afe568a073a2394d826c084f1878fd0ef19bd49/third_party/WebKit/Source/modules/sensor/Sensor.cpp [modify] https://crrev.com/0afe568a073a2394d826c084f1878fd0ef19bd49/third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp
,
Jun 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/13d0074ae638f5674c784e6568bacf5102d1df98 commit 13d0074ae638f5674c784e6568bacf5102d1df98 Author: vabr <vabr@chromium.org> Date: Mon Jun 05 10:31:46 2017 Don't move raw pointers in net Moving raw pointers does the same job as copying, but is more verbose and also more confusing: e.g., is the supposed pointer meant to be a smart one? This instance was flagged by the tool from https://codereview.chromium.org/2919243002/. BUG= 729393 Review-Url: https://codereview.chromium.org/2922883002 Cr-Commit-Position: refs/heads/master@{#476968} [modify] https://crrev.com/13d0074ae638f5674c784e6568bacf5102d1df98/net/quic/chromium/quic_chromium_client_session.cc [modify] https://crrev.com/13d0074ae638f5674c784e6568bacf5102d1df98/net/url_request/url_request_unittest.cc
,
Jun 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fe465afd8324f24de72a35c032da9ede5bb640f3 commit fe465afd8324f24de72a35c032da9ede5bb640f3 Author: vabr <vabr@chromium.org> Date: Mon Jun 05 13:21:35 2017 Don't move raw pointers in payments Moving raw pointers does the same job as copying, but is more verbose and also more confusing: e.g., is the supposed pointer meant to be a smart one? This instance was flagged by the tool from https://codereview.chromium.org/2919243002/. The raw pointer here actually contains a heap-allocated object, but the receiving member variable is already a raw pointer, so no automatic deletion happens currently. BUG= 729393 Review-Url: https://codereview.chromium.org/2919263002 Cr-Commit-Position: refs/heads/master@{#476975} [modify] https://crrev.com/fe465afd8324f24de72a35c032da9ede5bb640f3/chrome/browser/ui/views/payments/test_chrome_payment_request_delegate.cc
,
Jun 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/36da30c60297a992d61bfcc5065e6532adc62cf6 commit 36da30c60297a992d61bfcc5065e6532adc62cf6 Author: vabr <vabr@chromium.org> Date: Tue Jun 06 07:55:06 2017 Don't move raw pointers in content Moving raw pointers does the same job as copying, but is more verbose and also more confusing: e.g., is the supposed pointer meant to be a smart one? This instance was flagged by the tool from https://codereview.chromium.org/2919243002/. BUG= 729393 Review-Url: https://codereview.chromium.org/2923463002 Cr-Commit-Position: refs/heads/master@{#477229} [modify] https://crrev.com/36da30c60297a992d61bfcc5065e6532adc62cf6/content/browser/indexed_db/indexed_db_transaction.cc
,
Jun 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e38dbf83761a9f1ee952e4f908040d50b34f0b12 commit e38dbf83761a9f1ee952e4f908040d50b34f0b12 Author: vabr <vabr@chromium.org> Date: Tue Jun 06 08:23:35 2017 Don't move raw pointers in gpu Moving raw pointers does the same job as copying, but is more verbose and also more confusing: e.g., is the supposed pointer meant to be a smart one? This instance was flagged by the tool from https://codereview.chromium.org/2919243002/. BUG= 729393 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2920133002 Cr-Commit-Position: refs/heads/master@{#477233} [modify] https://crrev.com/e38dbf83761a9f1ee952e4f908040d50b34f0b12/gpu/ipc/service/gpu_channel.cc
,
Jun 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/75d70fe16e59fdacf28d65749cbc8c6f446a705a commit 75d70fe16e59fdacf28d65749cbc8c6f446a705a Author: vabr <vabr@chromium.org> Date: Tue Jun 06 10:02:00 2017 Don't move raw pointers in sync Moving raw pointers does the same job as copying, but is more verbose and also more confusing: e.g., is the supposed pointer meant to be a smart one? This instance was flagged by the tool from https://codereview.chromium.org/2919243002/. BUG= 729393 Review-Url: https://codereview.chromium.org/2923453002 Cr-Commit-Position: refs/heads/master@{#477250} [modify] https://crrev.com/75d70fe16e59fdacf28d65749cbc8c6f446a705a/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/75d70fe16e59fdacf28d65749cbc8c6f446a705a/components/sync_sessions/sessions_sync_manager.cc
,
Jun 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/beebf2b8c6344ff92cbb1e7079a75da479a7a9cd commit beebf2b8c6344ff92cbb1e7079a75da479a7a9cd Author: vabr <vabr@chromium.org> Date: Tue Jun 06 19:17:23 2017 Don't move raw pointers in services Moving raw pointers does the same job as copying, but is more verbose and also more confusing: e.g., is the supposed pointer meant to be a smart one? This instance was flagged by the tool from https://codereview.chromium.org/2919243002/. BUG= 729393 Review-Url: https://codereview.chromium.org/2919253002 Cr-Commit-Position: refs/heads/master@{#477365} [modify] https://crrev.com/beebf2b8c6344ff92cbb1e7079a75da479a7a9cd/services/preferences/tracked/pref_hash_store_impl.cc [modify] https://crrev.com/beebf2b8c6344ff92cbb1e7079a75da479a7a9cd/services/ui/public/cpp/bitmap/child_shared_bitmap_manager.cc [modify] https://crrev.com/beebf2b8c6344ff92cbb1e7079a75da479a7a9cd/services/video_capture/test/mock_device_factory.cc
,
Jun 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9ed3f43a6cc49b844d769498283b2e1e0be6cb94 commit 9ed3f43a6cc49b844d769498283b2e1e0be6cb94 Author: vabr <vabr@chromium.org> Date: Fri Jun 09 07:30:42 2017 Add a clang tool to detect std::move(raw ptr) The CL also adds a sentence to the documentation for clang tools, to supply a bit which was unclear for me during writing of the added tool. So far, on Linux, the tool found instances of std::move(raw ptr) in: ../../components/browser_sync/profile_sync_service.cc:10101 ../../components/sync_sessions/sessions_sync_manager.cc:5987 ../../content/browser/indexed_db/indexed_db_transaction.cc:3865 ../../gpu/ipc/service/gpu_channel.cc:26467 ../../services/preferences/tracked/pref_hash_store_impl.cc:3329 ../../services/preferences/tracked/pref_hash_store_impl.cc:4518 ../../services/ui/public/cpp/bitmap/child_shared_bitmap_manager.cc:5009 ../../services/video_capture/test/mock_device_factory.cc:2266 It also found it in some 3rd party headers, such as ../../third_party/flatbuffers/src/include/flatbuffers/flatbuffers.h:8260 ../../third_party/skia/include/core/SkCanvas.h:14756 ../../third_party/webrtc/api/proxy.h:16937 ../../third_party/webrtc/api/proxy.h:19681 ../../third_party/webrtc/api/proxy.h:20241 and c++ libs. The Chromium 1st party instances will be addressed in separate CLs. For the headers, I'll consider if it makes sense to address this (e.g., some occurrences seem to be template instantiations, where the template is not a priori aware that the supplied type is a raw pointer; in such cases std::move should stay). I may also run the tool on more platforms. Finally, while preventing regressions would be good, it seems impractical to run this tool as a presubmit check. However, I will try to run it after https://crbug.com/646113 is finished, because that base::Value refactoring might introduce some instances (happened to me recently). BUG= 729393 , 731577 Review-Url: https://codereview.chromium.org/2919243002 Cr-Commit-Position: refs/heads/master@{#478226} [modify] https://crrev.com/9ed3f43a6cc49b844d769498283b2e1e0be6cb94/docs/clang_tool_refactoring.md [add] https://crrev.com/9ed3f43a6cc49b844d769498283b2e1e0be6cb94/tools/clang/move_raw/CMakeLists.txt [add] https://crrev.com/9ed3f43a6cc49b844d769498283b2e1e0be6cb94/tools/clang/move_raw/MoveRaw.cpp [add] https://crrev.com/9ed3f43a6cc49b844d769498283b2e1e0be6cb94/tools/clang/move_raw/tests/test-expected.cc [add] https://crrev.com/9ed3f43a6cc49b844d769498283b2e1e0be6cb94/tools/clang/move_raw/tests/test-original.cc
,
Jun 9 2017
All what I found, and the tool itself, landed by now. I might check Mac and CrOS soon, and will associated fixing potential issues with this bug, but since most of the work is likely done, and the tool is finished, I'l mark this as fixed.
,
Jun 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/48e5a7f1adf55ec695f3c24bdf648db251b933d3 commit 48e5a7f1adf55ec695f3c24bdf648db251b933d3 Author: vabr <vabr@chromium.org> Date: Mon Jun 12 11:03:09 2017 Don't move raw pointers in HTMLCollection.h Moving raw pointers does the same job as copying, but is more verbose and also more confusing: e.g., is the supposed pointer meant to be a smart one? This instance was flagged by the tool from https://codereview.chromium.org/2919243002/. BUG= 729393 Review-Url: https://codereview.chromium.org/2927233003 Cr-Commit-Position: refs/heads/master@{#478589} [modify] https://crrev.com/48e5a7f1adf55ec695f3c24bdf648db251b933d3/third_party/WebKit/Source/core/html/HTMLCollection.h |
|||
►
Sign in to add a comment |
|||
Comment 1 by vabr@chromium.org
, Jun 4 2017