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

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

Created:
11 months ago by danakj
Modified:
11 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 #

Messages

Total messages: 76 (60 generated)
danakj
This might compile.
11 months 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 ...
11 months 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 ...
11 months ago (2016-12-20 14:27:08 UTC) #7
danakj
PTAL
11 months 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 ...
11 months 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. ...
11 months 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: ...
11 months 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 ...
11 months 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++, ...
11 months ago (2016-12-21 15:15:29 UTC) #48
danakj
Yep got it working.. pTAL
11 months 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 ...
11 months 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: ...
11 months ago (2016-12-21 20:58:38 UTC) #65
dcheng
lgtm, gl on the waterfall
11 months 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
11 months ago (2016-12-22 18:35:45 UTC) #71
commit-bot: I haz the power
Committed patchset #16 (id:300001)
11 months ago (2016-12-22 20:44:43 UTC) #74
commit-bot: I haz the power
11 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 efc10ee0f