Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1904663004:
SkTArray movable and swap for move only elements. (Closed)
|
Created:
4 years, 8 months ago by bungeman-skia Modified:
4 years, 8 months ago Reviewers:
mtklein CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkTArray movable and swap for move only elements.
SkTArray cannot currently contain move only elements because its swap
currently requires the SkTArray to be copyable. This makes SkTArray
movable and makes its swap move instead of copy.
Committed: https://skia.googlesource.com/skia/+/0d9e9bee17aa2901582c5461ae60f7241fc0cd59
Patch Set 1 #
Total comments: 2
Messages
Total messages: 17 (9 generated)
Description was changed from ========== SkTArray movable and swap for move only elements. SkTArray cannot currently contain move only elements because it's swap currently requires the SkTArray to be copyable. This makes SkTArray movable and makes it's swap move instead of copy. ========== to ========== SkTArray movable and swap for move only elements. SkTArray cannot currently contain move only elements because it's swap currently requires the SkTArray to be copyable. This makes SkTArray movable and makes it's swap move instead of copy. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== SkTArray movable and swap for move only elements. SkTArray cannot currently contain move only elements because it's swap currently requires the SkTArray to be copyable. This makes SkTArray movable and makes it's swap move instead of copy. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkTArray movable and swap for move only elements. SkTArray cannot currently contain move only elements because it's swap currently requires the SkTArray to be copyable. This makes SkTArray movable and makes it's swap move instead of copy. ==========
bungeman@google.com changed reviewers: + mtklein@google.com
Description was changed from ========== SkTArray movable and swap for move only elements. SkTArray cannot currently contain move only elements because it's swap currently requires the SkTArray to be copyable. This makes SkTArray movable and makes it's swap move instead of copy. ========== to ========== SkTArray movable and swap for move only elements. SkTArray cannot currently contain move only elements because its swap currently requires the SkTArray to be copyable. This makes SkTArray movable and makes it's swap move instead of copy. ==========
Description was changed from ========== SkTArray movable and swap for move only elements. SkTArray cannot currently contain move only elements because its swap currently requires the SkTArray to be copyable. This makes SkTArray movable and makes it's swap move instead of copy. ========== to ========== SkTArray movable and swap for move only elements. SkTArray cannot currently contain move only elements because its swap currently requires the SkTArray to be copyable. This makes SkTArray movable and makes its swap move instead of copy. ==========
lgtm https://codereview.chromium.org/1904663004/diff/1/include/private/SkTArray.h File include/private/SkTArray.h (right): https://codereview.chromium.org/1904663004/diff/1/include/private/SkTArray.h#... include/private/SkTArray.h:284: // This could be more optimal... Still true?
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/1904663004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904663004/1
The CQ bit was unchecked by bungeman@google.com
https://codereview.chromium.org/1904663004/diff/1/include/private/SkTArray.h File include/private/SkTArray.h (right): https://codereview.chromium.org/1904663004/diff/1/include/private/SkTArray.h#... include/private/SkTArray.h:284: // This could be more optimal... On 2016/04/20 15:50:44, mtklein wrote: > Still true? It could be. We could avoid dome destructor/constructor calls on the same memory by doing more moving. Also, if only one is using the pre-allocated array, it would be better to detach from the one that isn't, move from the pre-allocated to the non, then move from the detached to the pre-allocated. I think this means one less set of moves.
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/1904663004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904663004/1
Message was sent while issue was closed.
Description was changed from ========== SkTArray movable and swap for move only elements. SkTArray cannot currently contain move only elements because its swap currently requires the SkTArray to be copyable. This makes SkTArray movable and makes its swap move instead of copy. ========== to ========== SkTArray movable and swap for move only elements. SkTArray cannot currently contain move only elements because its swap currently requires the SkTArray to be copyable. This makes SkTArray movable and makes its swap move instead of copy. Committed: https://skia.googlesource.com/skia/+/0d9e9bee17aa2901582c5461ae60f7241fc0cd59 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/0d9e9bee17aa2901582c5461ae60f7241fc0cd59
Message was sent while issue was closed.
Early warning: this CL has broken 'CFI Linux' buildbot: https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/5115 Error message: ../../third_party/skia/include/gpu/../private/SkTArray.h:189:19: runtime error: control flow integrity check for type 'GrGLSampler' failed during cast to unrelated type (vtable address 0x4242424242424242) 0x4242424242424242: note: invalid vtable Stack trace: #0 0x00000000004d1fa4 in __ubsan_handle_cfi_check_fail () at ../../third_party/skia/src/gpu/glsl/GrGLSLSampler.h:17 #1 0x00000000010d2b4a in GrGLSampler& SkTArray<GrGLSampler, false>::emplace_back<unsigned int&, GrPixelConfig&, GrSLType&, GrSLPrecision&, char const*>(unsigned int&, GrPixelConfig&, GrSLType&, GrSLPrecision&, char const*&&) () at ../../third_party/skia/include/gpu/../private/SkTArray.h:189 #2 0x00000000010d29c9 in GrGLUniformHandler::internalAddSampler(unsigned int, GrPixelConfig, GrSLType, GrSLPrecision, char const*) () at ../../third_party/skia/src/gpu/gl/GrGLUniformHandler.cpp:67 #3 0x00000000010dd087 in GrGLSLProgramBuilder::emitSampler(GrSLType, GrPixelConfig, char const*, GrShaderFlags, SkTArray<GrGLSLProgramDataManager::ShaderResourceHandle, false>*) () at ../../third_party/skia/src /gpu/glsl/GrGLSLProgramBuilder.cpp:279 A proper bug report is to follow.
Message was sent while issue was closed.
Filed https://crbug.com/605337 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
