Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(43)

Issue 1466833002: C++11 atomics (Closed)

Created:
5 years, 1 month ago by JF
Modified:
5 years ago
Reviewers:
Nico
CC:
chromium-reviews, vmpstr+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

C++11 atomics Use only the 'portable' atomics when possible [*], which use C++11 atomics. After this patch, individual uses of atomicops.h, for all platforms, should be migrated on by one to explicitly use std::atomic. This will require adapting code which has different assumptions (not all the memory accesses are the same, std::atomics does have signed and unsigned integer both without UB, volatile atomic should be used with shared memory, etc). [*] Portable atomics aren't used on Windows and OSX for now: some implicit inclusions of windows.h need to be fixed when moving to <atomic> (it was included through "atomicops.h"); some compatibility issues need to be fixed on OSX (see compat.h). R= thakis@chromium.org BUG=559247 TEST= git cl try Committed: https://crrev.com/a8427ea85054142d590508388433b1fdc9451970 Cr-Commit-Position: refs/heads/master@{#363758}

Patch Set 1 #

Patch Set 2 : Don't change Windows yet. #

Patch Set 3 : Keep compat.h for now. #

Total comments: 5

Patch Set 4 : Address thakis' comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -221 lines) Patch
M base/BUILD.gn View 1 2 chunks +0 lines, -2 lines 0 comments Download
M base/atomicops.h View 1 2 3 1 chunk +6 lines, -20 lines 0 comments Download
D base/atomicops_internals_mac.h View 1 chunk +0 lines, -197 lines 0 comments Download
M base/base.gypi View 1 2 chunks +0 lines, -2 lines 0 comments Download

Messages

Total messages: 20 (7 generated)
JF
This is a first attempt, let's see if the bots like me.
5 years, 1 month ago (2015-11-20 21:32:42 UTC) #1
Nico
Thanks for looking at this! Looks like iOS is unhappy about something that looks fixable. ...
5 years, 1 month ago (2015-11-20 23:04:50 UTC) #2
JF
As discussed here: https://groups.google.com/a/chromium.org/d/msg/cxx/Ej3RAiaI44s/G1qfsKZWBQAJ It looks like there are a few issues on Windows. All ...
5 years ago (2015-12-06 20:36:32 UTC) #3
JF
It looks like iOS builds depend on base/atomicops_internals_atomicword_compat.h, and the breakage happens in ./base/memory/singleton.h which ...
5 years ago (2015-12-06 21:25:41 UTC) #4
JF
The Linux failure looks unrelated, rest passes.
5 years ago (2015-12-07 01:00:12 UTC) #5
Nico
nice, lgtm! Please update the CL description to say that this is not on all ...
5 years ago (2015-12-07 03:55:58 UTC) #6
JF
https://codereview.chromium.org/1466833002/diff/40001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/1466833002/diff/40001/base/atomicops.h#newcode147 base/atomicops.h:147: #if (defined(OS_WIN) && defined(COMPILER_MSVC) && \ On 2015/12/07 03:55:58, ...
5 years ago (2015-12-07 04:23:31 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466833002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466833002/60001
5 years ago (2015-12-07 04:24:10 UTC) #11
Nico
https://codereview.chromium.org/1466833002/diff/40001/base/atomicops.h File base/atomicops.h (right): https://codereview.chromium.org/1466833002/diff/40001/base/atomicops.h#newcode147 base/atomicops.h:147: #if (defined(OS_WIN) && defined(COMPILER_MSVC) && \ On 2015/12/07 04:23:31, ...
5 years ago (2015-12-07 04:58:28 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/151183)
5 years ago (2015-12-07 06:21:56 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1466833002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1466833002/60001
5 years ago (2015-12-08 06:49:03 UTC) #16
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years ago (2015-12-08 08:58:25 UTC) #18
commit-bot: I haz the power
5 years ago (2015-12-08 08:59:13 UTC) #20
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/a8427ea85054142d590508388433b1fdc9451970
Cr-Commit-Position: refs/heads/master@{#363758}

Powered by Google App Engine
This is Rietveld 408576698