Chromium Code Reviews
Help | Chromium Project | Sign in
(23)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by danakj
Modified:
3 months, 1 week 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.
3 months, 1 week 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 ...
3 months, 1 week 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 ...
3 months, 1 week ago (2016-12-20 14:27:08 UTC) #7
danakj
PTAL
3 months, 1 week 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 ...
3 months, 1 week 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. ...
3 months, 1 week 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: ...
3 months, 1 week 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 ...
3 months, 1 week 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++, ...
3 months, 1 week ago (2016-12-21 15:15:29 UTC) #48
danakj
Yep got it working.. pTAL
3 months, 1 week 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 ...
3 months, 1 week 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: ...
3 months, 1 week ago (2016-12-21 20:58:38 UTC) #65
dcheng
lgtm, gl on the waterfall
3 months, 1 week 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 months, 1 week ago (2016-12-22 18:35:45 UTC) #71
commit-bot: I haz the power
Committed patchset #16 (id:300001)
3 months, 1 week ago (2016-12-22 20:44:43 UTC) #74
commit-bot: I haz the power
3 months, 1 week 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}
Sign in to reply to this message.

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