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

Issue 2583353002: Make bit_cast fail if the source or dest are not trivially copyable (Closed)

Created:
4 years ago by danakj
Modified:
3 years, 12 months ago
Reviewers:
dcheng
CC:
chromium-reviews, vmpstr+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Introduce base::is_trivially_copyable and use for bit_cast bit_cast had some one-off preprocessor logic to check or not to check for the types being trivially copyable that ended up not being enabled at all on windows. Since most of our platforms are now C++11 make that the norm and cover the various edge cases via base::is_trivially_copyable. Then bit_cast can use that and will be able to static_assert for this on all platforms. R=dcheng BUG=555754 Committed: https://crrev.com/7cebcfb2d80152bc84d88fc711eda6e61acda8be Cr-Commit-Position: refs/heads/master@{#440491}

Patch Set 1 #

Patch Set 2 #

Total comments: 1

Patch Set 3 : bitcast: tryagain #

Patch Set 4 : bitcast: const-bool #

Patch Set 5 : bitcast: constexpr-bool #

Patch Set 6 : bitcast: static #

Patch Set 7 : bitcast: trystuff #

Patch Set 8 : bitcast: pragma #

Patch Set 9 : bitcast: warning #

Patch Set 10 : bitcast: tryagain #

Patch Set 11 : bitcast: maybework #

Patch Set 12 : bitcast: fidefine #

Patch Set 13 : bitcast: chromeos-pls #

Patch Set 14 : bitcast: really-gcc #

Patch Set 15 : bitcast: . #

Total comments: 6

Patch Set 16 : bitcast: fixes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -29 lines) Patch
M base/bit_cast.h View 1 2 2 chunks +5 lines, -28 lines 0 comments Download
M base/bit_cast_unittest.cc View 1 1 chunk +0 lines, -1 line 0 comments Download
M base/template_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +62 lines, -0 lines 0 comments Download

Messages

Total messages: 76 (60 generated)
danakj
This might compile.
4 years ago (2016-12-19 22:14:00 UTC) #3
dcheng
On 2016/12/19 22:14:00, danakj_OOO_and_slow wrote: > This might compile. It didn't (which I think is ...
4 years ago (2016-12-19 23:57:21 UTC) #6
danakj
On 2016/12/19 23:57:21, dcheng wrote: > On 2016/12/19 22:14:00, danakj_OOO_and_slow wrote: > > This might ...
4 years ago (2016-12-20 14:27:08 UTC) #7
danakj
PTAL
4 years ago (2016-12-20 14:29:02 UTC) #8
danakj
https://codereview.chromium.org/2583353002/diff/20001/base/bit_cast.h File base/bit_cast.h (right): https://codereview.chromium.org/2583353002/diff/20001/base/bit_cast.h#newcode77 base/bit_cast.h:77: static_assert(std::is_trivially_copyable<Dest>::value, Wait we already have this. With crazy stuff ...
4 years ago (2016-12-20 16:40:49 UTC) #13
danakj
I have removed the #ifs from bit_cast.h and moved to something simpler in template_util.h instead. ...
4 years ago (2016-12-20 17:00:56 UTC) #17
danakj
So this is exciting. On android we get this: In file included from ../../base/bit_cast_unittest.cc:6:0: ../../base/bit_cast.h: ...
4 years ago (2016-12-20 18:35:46 UTC) #28
danakj
On chromeos we get: /b/c/cipd/goma/gomacc armv7a-cros-linux-gnueabi-g++ -B/b/c/b/chromeos_daisy_chromium_compile_only_ng/.cros_cache/chrome-sdk/tarballs/daisy+9095.0.0+target_toolchain/usr/x86_64-pc-linux-gnu/armv7a-cros-linux-gnueabi/binutils-bin/2.25.51-gold -MMD -MF obj/ash/public/interfaces/interfaces_shared_cpp_sources/accelerator_controller.mojom-shared.o.d -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_ASH=1 -DUSE_AURA=1 ...
4 years ago (2016-12-20 18:37:07 UTC) #29
danakj
Ok so whats happening here is I believe: 1. Android is using clang with libstdc++, ...
4 years ago (2016-12-21 15:15:29 UTC) #48
danakj
Yep got it working.. pTAL
4 years ago (2016-12-21 17:24:25 UTC) #54
dcheng
Use a sysroot, they said. Just one standard library to program against, they said. https://codereview.chromium.org/2583353002/diff/280001/base/template_util.h ...
4 years ago (2016-12-21 20:50:18 UTC) #63
danakj
PTAL https://codereview.chromium.org/2583353002/diff/280001/base/template_util.h File base/template_util.h (right): https://codereview.chromium.org/2583353002/diff/280001/base/template_util.h#newcode29 base/template_util.h:29: #define CR_GLIBCXX_5_0_0 20150123 On 2016/12/21 20:50:18, dcheng wrote: ...
4 years ago (2016-12-21 20:58:38 UTC) #65
dcheng
lgtm, gl on the waterfall
4 years ago (2016-12-21 22:34:43 UTC) #67
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/2583353002/300001
3 years, 12 months ago (2016-12-22 18:35:45 UTC) #71
commit-bot: I haz the power
Committed patchset #16 (id:300001)
3 years, 12 months ago (2016-12-22 20:44:43 UTC) #74
commit-bot: I haz the power
3 years, 12 months ago (2016-12-22 20:48:20 UTC) #76
Message was sent while issue was closed.
Patchset 16 (id:??) landed as
https://crrev.com/7cebcfb2d80152bc84d88fc711eda6e61acda8be
Cr-Commit-Position: refs/heads/master@{#440491}

Powered by Google App Engine
This is Rietveld 408576698