Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 2932053002: Use C++11 alignment primitives (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by brettw
Modified:
2 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, kinuko+watch, vmpstr+watch_chromium.org, gavinp+memory_chromium.org, net-reviews_chromium.org, asvitkine+watch_chromium.org, blink-reviews, cc-bugs_chromium.org, danakj+watch_chromium.org, jln+watch_chromium.org, Mark Mentovai
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Use C++11 alignment primitives Removes the compiler-specific base #define ALIGNOF and replace uses with alignof. Replaces some uses of ALIGNAS with alignas. These are not all replaced, and a note is added to the definition about the problems with alignas. This came from https://codereview.chromium.org/2670873002/ Remove base::AlignedMemory and replace with alignas(type) char[size]. std::aligned_storage has some limitations. The style guide is updated to note these and mark it disallowed. It is also updated for alignas and alignof. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2932053002 Cr-Commit-Position: refs/heads/master@{#479169} Committed: https://chromium.googlesource.com/chromium/src/+/16289b3ef759d892a4058671e0657c016101be23

Patch Set 1 #

Patch Set 2 : Linux Chrome #

Patch Set 3 : alignas #

Patch Set 4 : Format #

Total comments: 1

Patch Set 5 : Put back ALIGNAS #

Total comments: 6

Patch Set 6 : No aligned_storage #

Patch Set 7 : Fixes #

Total comments: 2

Patch Set 8 : Merge #

Patch Set 9 : Fixes #

Patch Set 10 : Merge #

Patch Set 11 : Merge #

Patch Set 12 : Fix merge #

Unified diffs Side-by-side diffs Delta from patch set Stats (+166 lines, -253 lines) Patch
M base/compiler_specific.h View 1 2 3 4 5 6 7 8 1 chunk +17 lines, -9 lines 0 comments Download
M base/containers/stack_container.h View 1 2 3 4 5 6 2 chunks +4 lines, -5 lines 0 comments Download
M base/containers/stack_container_unittest.cc View 1 2 3 4 5 6 3 chunks +4 lines, -5 lines 0 comments Download
M base/lazy_instance.h View 1 2 3 4 5 6 7 4 chunks +17 lines, -6 lines 0 comments Download
M base/lazy_instance_unittest.cc View 1 2 3 4 5 2 chunks +3 lines, -3 lines 0 comments Download
M base/memory/aligned_memory.h View 2 chunks +4 lines, -64 lines 0 comments Download
M base/memory/aligned_memory_unittest.cc View 1 chunk +12 lines, -61 lines 0 comments Download
M base/memory/manual_constructor.h View 1 2 3 4 5 6 3 chunks +5 lines, -7 lines 0 comments Download
M base/memory/singleton.h View 1 2 3 4 5 6 7 8 3 chunks +3 lines, -5 lines 0 comments Download
M base/memory/singleton_unittest.cc View 1 2 3 4 5 2 chunks +14 lines, -8 lines 0 comments Download
M base/metrics/persistent_memory_allocator.h View 2 chunks +2 lines, -2 lines 0 comments Download
M base/process/launch_posix.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M cc/base/list_container.h View 3 chunks +3 lines, -3 lines 0 comments Download
M cc/base/list_container_unittest.cc View 15 chunks +16 lines, -16 lines 0 comments Download
M cc/output/renderer_pixeltest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M cc/paint/paint_op_buffer.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +4 lines, -4 lines 0 comments Download
M cc/paint/paint_op_buffer.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M cc/quads/largest_draw_quad.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M cc/quads/render_pass.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M cc/raster/texture_compressor_etc1.h View 1 2 3 1 chunk +4 lines, -9 lines 0 comments Download
M cc/raster/texture_compressor_etc1_sse.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M components/tracing/core/proto_zero_message.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/media/audio_input_sync_writer_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc View 1 1 chunk +0 lines, -1 line 0 comments Download
M media/base/audio_bus_unittest.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M media/base/video_frame.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M sandbox/linux/services/credentials.cc View 1 2 3 4 6 7 1 chunk +1 line, -1 line 0 comments Download
M styleguide/c++/c++11.html View 1 2 3 4 5 6 7 4 chunks +18 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/platform/wtf/StdLibExtras.h View 1 2 3 4 5 6 2 chunks +3 lines, -4 lines 0 comments Download
M third_party/crashpad/crashpad/util/stdlib/aligned_allocator.h View 3 chunks +2 lines, -3 lines 0 comments Download
M third_party/crashpad/crashpad/util/stdlib/aligned_allocator_test.cc View 1 2 2 chunks +16 lines, -16 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 102 (54 generated)
brettw
Linux Chrome
2 months, 1 week ago (2017-06-09 19:10:39 UTC) #6
brettw
alignas
2 months, 1 week ago (2017-06-09 19:35:56 UTC) #11
brettw
Format
2 months, 1 week ago (2017-06-09 19:38:27 UTC) #12
brettw
scottmg: third_party/crashpad danakj: everything else (it's mostly base and cc)
2 months, 1 week ago (2017-06-09 19:42:42 UTC) #15
scottmg
https://codereview.chromium.org/2932053002/diff/60001/third_party/crashpad/crashpad/minidump/minidump_extensions.h File third_party/crashpad/crashpad/minidump/minidump_extensions.h (right): https://codereview.chromium.org/2932053002/diff/60001/third_party/crashpad/crashpad/minidump/minidump_extensions.h#newcode105 third_party/crashpad/crashpad/minidump/minidump_extensions.h:105: struct alignas(4) PACKED MinidumpUTF8String { There's a bunch more ...
2 months, 1 week ago (2017-06-09 19:49:01 UTC) #19
Mark Mentovai
Note a previous attempt to do this months ago, https://codereview.chromium.org/2670873002/.
2 months, 1 week ago (2017-06-09 20:03:16 UTC) #21
brettw
:( I wasn't aware of the previous attempt. I'll trim this down (at least we ...
2 months, 1 week ago (2017-06-09 20:29:23 UTC) #25
danakj
On Fri, Jun 9, 2017 at 4:29 PM, <brettw@chromium.org> wrote: > :( I wasn't aware ...
2 months, 1 week ago (2017-06-09 20:34:32 UTC) #26
danakj
On Fri, Jun 9, 2017 at 4:29 PM, <brettw@chromium.org> wrote: > :( I wasn't aware ...
2 months, 1 week ago (2017-06-09 20:41:59 UTC) #27
brettw
Put back ALIGNAS
2 months, 1 week ago (2017-06-10 15:29:56 UTC) #28
brettw
Okay, I put back the ALIGNAS definition and kept using it in a few cases ...
2 months, 1 week ago (2017-06-10 15:32:07 UTC) #30
brettw
+dcheng FYI who wrote the last patch.
2 months, 1 week ago (2017-06-10 17:27:46 UTC) #34
dcheng
Note that the reason I never landed the previous patch is thakis@ felt there were ...
2 months, 1 week ago (2017-06-10 17:43:17 UTC) #37
Nico
On 2017/06/10 17:43:17, dcheng wrote: > Note that the reason I never landed the previous ...
2 months, 1 week ago (2017-06-10 17:58:23 UTC) #39
dcheng
On 2017/06/10 17:58:23, Nico (vacation Jun 3-11) wrote: > On 2017/06/10 17:43:17, dcheng wrote: > ...
2 months, 1 week ago (2017-06-10 18:15:33 UTC) #40
brettw
There seems to be some problem on Windows with aligned_storage I need to track down ...
2 months ago (2017-06-11 16:02:11 UTC) #41
danakj
cc and base LGTM https://codereview.chromium.org/2932053002/diff/80001/base/compiler_specific.h File base/compiler_specific.h (right): https://codereview.chromium.org/2932053002/diff/80001/base/compiler_specific.h#newcode129 base/compiler_specific.h:129: // these attributes. This means ...
2 months ago (2017-06-12 18:32:10 UTC) #42
brettw
Exciting times: std::aligned_storage won't work for anything larger than 8 in MS's STL implementation. It's ...
2 months ago (2017-06-12 19:34:47 UTC) #43
brettw
No aligned_storage
2 months ago (2017-06-12 21:41:33 UTC) #44
brettw
Fixes
2 months ago (2017-06-12 21:54:32 UTC) #45
brettw
Okay, the new version has no aligned_storage. This should be pretty trivial. The main difference ...
2 months ago (2017-06-12 21:56:03 UTC) #47
brettw
Merge
2 months ago (2017-06-12 22:17:13 UTC) #52
danakj
https://codereview.chromium.org/2932053002/diff/120001/base/memory/singleton_unittest.cc File base/memory/singleton_unittest.cc (right): https://codereview.chromium.org/2932053002/diff/120001/base/memory/singleton_unittest.cc#newcode292 base/memory/singleton_unittest.cc:292: EXPECT_ALIGNED(align4, 4); Is this legit? I realize AlignedData has ...
2 months ago (2017-06-12 22:22:05 UTC) #55
brettw
https://codereview.chromium.org/2932053002/diff/80001/base/compiler_specific.h File base/compiler_specific.h (right): https://codereview.chromium.org/2932053002/diff/80001/base/compiler_specific.h#newcode129 base/compiler_specific.h:129: // these attributes. This means that it is not ...
2 months ago (2017-06-12 22:22:15 UTC) #56
danakj
https://codereview.chromium.org/2932053002/diff/80001/base/containers/stack_container_unittest.cc File base/containers/stack_container_unittest.cc (right): https://codereview.chromium.org/2932053002/diff/80001/base/containers/stack_container_unittest.cc#newcode112 base/containers/stack_container_unittest.cc:112: typename std::aligned_storage<alignment, alignment>::type data_; On 2017/06/12 22:22:15, brettw wrote: ...
2 months ago (2017-06-12 22:23:10 UTC) #57
brettw
https://codereview.chromium.org/2932053002/diff/120001/base/memory/singleton_unittest.cc File base/memory/singleton_unittest.cc (right): https://codereview.chromium.org/2932053002/diff/120001/base/memory/singleton_unittest.cc#newcode292 base/memory/singleton_unittest.cc:292: EXPECT_ALIGNED(align4, 4); On 2017/06/12 22:22:05, danakj wrote: > Is ...
2 months ago (2017-06-12 22:28:50 UTC) #58
brettw
Fixes
2 months ago (2017-06-12 22:44:45 UTC) #61
danakj
On Mon, Jun 12, 2017 at 6:28 PM, <brettw@chromium.org> wrote: > > https://codereview.chromium.org/2932053002/diff/120001/ > base/memory/singleton_unittest.cc ...
2 months ago (2017-06-12 23:33:27 UTC) #64
danakj
On Mon, Jun 12, 2017 at 6:28 PM, <brettw@chromium.org> wrote: > > https://codereview.chromium.org/2932053002/diff/120001/ > base/memory/singleton_unittest.cc ...
2 months ago (2017-06-12 23:33:27 UTC) #65
danakj
LGTM otherwise, incl styleguide
2 months ago (2017-06-12 23:33:56 UTC) #66
brettw
On 2017/06/12 23:33:27, danakj wrote: > On Mon, Jun 12, 2017 at 6:28 PM, <mailto:brettw@chromium.org> ...
2 months ago (2017-06-13 04:43:57 UTC) #69
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2932053002/160001
2 months ago (2017-06-13 04:44:24 UTC) #72
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/462101)
2 months ago (2017-06-13 04:53:29 UTC) #74
(unused - use chromium)
mark, can you lg-stamp crashpad?
2 months ago (2017-06-13 13:13:34 UTC) #76
danakj
On Tue, Jun 13, 2017 at 12:43 AM, <brettw@chromium.org> wrote: > On 2017/06/12 23:33:27, danakj ...
2 months ago (2017-06-13 14:30:52 UTC) #77
danakj
On Tue, Jun 13, 2017 at 12:43 AM, <brettw@chromium.org> wrote: > On 2017/06/12 23:33:27, danakj ...
2 months ago (2017-06-13 14:30:52 UTC) #78
Mark Mentovai
LGTM in crashpad, and we’ll carry this upstream.
2 months ago (2017-06-13 14:46:11 UTC) #79
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2932053002/160001
2 months ago (2017-06-13 14:49:58 UTC) #81
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/462453)
2 months ago (2017-06-13 14:59:44 UTC) #83
brettw
Merge
2 months ago (2017-06-13 18:08:25 UTC) #84
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2932053002/180001
2 months ago (2017-06-13 18:09:07 UTC) #87
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/236562) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, ...
2 months ago (2017-06-13 18:12:42 UTC) #89
brettw
Merge
2 months ago (2017-06-13 18:22:31 UTC) #90
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2932053002/200001
2 months ago (2017-06-13 18:23:19 UTC) #93
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_headless_rel/builds/106133)
2 months ago (2017-06-13 18:37:49 UTC) #95
brettw
Fix merge
2 months ago (2017-06-13 19:08:44 UTC) #96
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2932053002/220001
2 months ago (2017-06-13 19:09:35 UTC) #99
commit-bot: I haz the power
2 months ago (2017-06-13 21:59:04 UTC) #102
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/16289b3ef759d892a4058671e065...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b