Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1672063002:
SkTArray to move when moving. (Closed)
|
Created:
4 years, 10 months ago by bungeman-skia Modified:
4 years, 10 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkTArray to move when moving.
This updates SkTArray to move elements when possible, instead of always
copying them.
TBR=reed
Agreed moving is good. This should also become private.
Committed: https://skia.googlesource.com/skia/+/3c69348e725131150e4ab962dec1b3ff1148a6bd
Committed: https://skia.googlesource.com/skia/+/918090c819109585f8fd2295039385a60eb0b572
Patch Set 1 #Patch Set 2 : Take advantage of more c++11 to make things easier. #
Total comments: 4
Patch Set 3 : Clarify comment. #Patch Set 4 : Disable bad language extensions, fix existing code. #
Messages
Total messages: 45 (25 generated)
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
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/1672063002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672063002/1
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. ==========
bungeman@google.com changed reviewers: + mtklein@google.com, reed@google.com
Came up while working on another CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. ==========
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/1672063002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672063002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1672063002/diff/20001/include/core/SkTArray.h File include/core/SkTArray.h (right): https://codereview.chromium.org/1672063002/diff/20001/include/core/SkTArray.h... include/core/SkTArray.h:394: * When moving (and MEM_COPY is false), the moved from element will be destroyed. Shouldn't we just move assign/copy it and let the type of the moved-from element handle its own business?
https://codereview.chromium.org/1672063002/diff/20001/include/core/SkTArray.h File include/core/SkTArray.h (right): https://codereview.chromium.org/1672063002/diff/20001/include/core/SkTArray.h... include/core/SkTArray.h:394: * When moving (and MEM_COPY is false), the moved from element will be destroyed. On 2016/02/07 22:21:23, mtklein wrote: > Shouldn't we just move assign/copy it and let the type of the moved-from element > handle its own business? How would it? Everything here is placement newed and destructed, it's not like we ever call delete on these. Even moved-from objects need to be destroyed. Also, this is just documenting what was already happening, but it wasn't clear before what the real intent was (this makes the two 'move' methods more obviously parallel). Perhaps instead of calling these methods 'move' they could be called 'moveAndDestroy' to avoid confusion with std::move(InputIt, InputIt, OutputIt).
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/1672063002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672063002/20001
https://codereview.chromium.org/1672063002/diff/20001/include/core/SkTArray.h File include/core/SkTArray.h (right): https://codereview.chromium.org/1672063002/diff/20001/include/core/SkTArray.h... include/core/SkTArray.h:394: * When moving (and MEM_COPY is false), the moved from element will be destroyed. On 2016/02/07 22:21:23, mtklein wrote: > Shouldn't we just move assign/copy it and let the type of the moved-from element > handle its own business? Perhaps it would be clearer to replace this second sentence with: In the following move methods, 'src' is destroyed leaving behind uninitialized raw storage.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1672063002/diff/20001/include/core/SkTArray.h File include/core/SkTArray.h (right): https://codereview.chromium.org/1672063002/diff/20001/include/core/SkTArray.h... include/core/SkTArray.h:394: * When moving (and MEM_COPY is false), the moved from element will be destroyed. On 2016/02/08 03:36:24, bungeman1 wrote: > On 2016/02/07 22:21:23, mtklein wrote: > > Shouldn't we just move assign/copy it and let the type of the moved-from > element > > handle its own business? > > Perhaps it would be clearer to replace this second sentence with: > > In the following move methods, 'src' is destroyed leaving behind uninitialized > raw storage. sgtm
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. ==========
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. TBR=reed Agreed moving is good. This should also become private. ==========
The CQ bit was checked by bungeman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/1672063002/#ps40001 (title: "Clarify comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1672063002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672063002/40001
Message was sent while issue was closed.
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. TBR=reed Agreed moving is good. This should also become private. ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. TBR=reed Agreed moving is good. This should also become private. Committed: https://skia.googlesource.com/skia/+/3c69348e725131150e4ab962dec1b3ff1148a6bd ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/3c69348e725131150e4ab962dec1b3ff1148a6bd
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1677103002/ by kjlubick@google.com. The reason for reverting is: This appears to have broken several things..
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1683693002/ by mtklein@google.com. The reason for reverting is: Broke the Chrome roll: https://codereview.chromium.org/1680563005.
Message was sent while issue was closed.
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. TBR=reed Agreed moving is good. This should also become private. Committed: https://skia.googlesource.com/skia/+/3c69348e725131150e4ab962dec1b3ff1148a6bd ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. TBR=reed Agreed moving is good. This should also become private. Committed: https://skia.googlesource.com/skia/+/3c69348e725131150e4ab962dec1b3ff1148a6bd ==========
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. TBR=reed Agreed moving is good. This should also become private. Committed: https://skia.googlesource.com/skia/+/3c69348e725131150e4ab962dec1b3ff1148a6bd ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. TBR=reed Agreed moving is good. This should also become private. Committed: https://skia.googlesource.com/skia/+/3c69348e725131150e4ab962dec1b3ff1148a6bd GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. TBR=reed Agreed moving is good. This should also become private. Committed: https://skia.googlesource.com/skia/+/3c69348e725131150e4ab962dec1b3ff1148a6bd GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. TBR=reed Agreed moving is good. This should also become private. Committed: https://skia.googlesource.com/skia/+/3c69348e725131150e4ab962dec1b3ff1148a6bd ==========
Agitated for clang-cl trybot for Skia and auto-roller, added warnings as errors to prevent this in the future (having done so previously would have caught this problem).
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/1672063002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672063002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bungeman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/1672063002/#ps60001 (title: "Disable bad language extensions, fix existing code.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1672063002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672063002/60001
Message was sent while issue was closed.
Description was changed from ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. TBR=reed Agreed moving is good. This should also become private. Committed: https://skia.googlesource.com/skia/+/3c69348e725131150e4ab962dec1b3ff1148a6bd ========== to ========== SkTArray to move when moving. This updates SkTArray to move elements when possible, instead of always copying them. TBR=reed Agreed moving is good. This should also become private. Committed: https://skia.googlesource.com/skia/+/3c69348e725131150e4ab962dec1b3ff1148a6bd Committed: https://skia.googlesource.com/skia/+/918090c819109585f8fd2295039385a60eb0b572 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/918090c819109585f8fd2295039385a60eb0b572 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
