bit_cast: check is_trivially_copyable

bit_cast: check is_trivially_copyable

Instead of using a misleading comment about POD-ness.

R=thakis@chromium.org

Committed: https://crrev.com/d81c1ced74e63e4bf50019ffd2d398def71bdae7
Cr-Commit-Position: refs/heads/master@{#385281}

[JF, 2016-03-25 20:27:56]

The CQ bit was checked by jfb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-25 20:28:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1837563002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1837563002/1

[commit-bot: I haz the power, 2016-03-25 21:39:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-25 21:39:47]

Dry run: This issue passed the CQ dry run.

[JF, 2016-03-30 22:27:55]

Ping.

[Nico, 2016-03-30 22:42:34]

I don't think you need the libc++ version check -- we don't support
building with arbitrary libc++s and the ones we do support should be new
enough.

The "clang claims GCC" comment isn't true on Windows. Speaking of, what's
the windows story for this?
On Mar 30, 2016 6:27 PM, <jfb@chromium.org> wrote:

> Ping.
>
> https://codereview.chromium.org/1837563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[JF, 2016-03-30 22:52:20]

On 2016/03/30 22:42:34, Nico wrote:
> I don't think you need the libc++ version check -- we don't support
> building with arbitrary libc++s and the ones we do support should be new
> enough.

I can remove it if you want. I put it there because it would be nice for this
code to be correct in non-Chrome circumstances since base/ gets copied around
quite a bit.

> The "clang claims GCC" comment isn't true on Windows. Speaking of, what's
> the windows story for this?

Strictly better than it is now: the non-Windows trybots give you an error.
AFAICT there doesn't seem to be a good option for MSVS, but I may be wrong on
this.

[Nico, 2016-03-31 01:07:19]

On 2016/03/30 22:52:20, JF wrote:
> On 2016/03/30 22:42:34, Nico wrote:
> > I don't think you need the libc++ version check -- we don't support
> > building with arbitrary libc++s and the ones we do support should be new
> > enough.
> 
> I can remove it if you want. I put it there because it would be nice for this
> code to be correct in non-Chrome circumstances since base/ gets copied around
> quite a bit.

That's something we very explicitly don't support.

> > The "clang claims GCC" comment isn't true on Windows. Speaking of, what's
> > the windows story for this?
> 
> Strictly better than it is now: the non-Windows trybots give you an error.
> AFAICT there doesn't seem to be a good option for MSVS, but I may be wrong on
> this.

Does std::is_trivially_copyable not work there? I guess with clang-cl you can
use the intrinsic?

[JF, 2016-03-31 21:08:07]

On 2016/03/31 01:07:19, Nico wrote:
> On 2016/03/30 22:52:20, JF wrote:
> > On 2016/03/30 22:42:34, Nico wrote:
> > > I don't think you need the libc++ version check -- we don't support
> > > building with arbitrary libc++s and the ones we do support should be new
> > > enough.
> > 
> > I can remove it if you want. I put it there because it would be nice for
this
> > code to be correct in non-Chrome circumstances since base/ gets copied
around
> > quite a bit.
> 
> That's something we very explicitly don't support.

OK, changed.
 
> > > The "clang claims GCC" comment isn't true on Windows. Speaking of, what's
> > > the windows story for this?
> > 
> > Strictly better than it is now: the non-Windows trybots give you an error.
> > AFAICT there doesn't seem to be a good option for MSVS, but I may be wrong
on
> > this.
> 
> Does std::is_trivially_copyable not work there? I guess with clang-cl you can
> use the intrinsic?

I'm not sure why, but it took a while to be in most STL implementations. That's
why this isn't pretty :-)

I updated the code so that clang-cl will use the right intrinsic (and GCC still
falls back to the older one). This still all falls back to the bots in the worst
case.

[JF, 2016-03-31 21:08:15]

The CQ bit was checked by jfb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-31 21:09:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1837563002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1837563002/20001

[commit-bot: I haz the power, 2016-03-31 21:23:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-31 21:23:22]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Nico, 2016-03-31 22:28:15]

lgtm with obvious change

https://codereview.chromium.org/1837563002/diff/20001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/1837563002/diff/20001/base/bit_cast.h#newcode74
base/bit_cast.h:74: #elif defined(__has_feature) &&
__has_feature(is_trivially_copyable)
this has to be on two lines:

../../base/bit_cast.h:74:46: error: missing binary operator before token "("
 #elif defined(__has_feature) && __has_feature(is_trivially_copyable)

[JF, 2016-03-31 22:49:55]

The CQ bit was checked by jfb@chromium.org to run a CQ dry run

[JF, 2016-03-31 22:50:17]

PTAL at the updated change, I think handling __has_feature this way is cleaner.

[commit-bot: I haz the power, 2016-03-31 22:50:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1837563002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1837563002/40001

[commit-bot: I haz the power, 2016-04-01 06:48:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-01 06:48:55]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[JF, 2016-04-01 16:28:24]

The CQ bit was checked by jfb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-01 16:28:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1837563002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1837563002/40001

[commit-bot: I haz the power, 2016-04-01 18:14:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-01 18:14:58]

Dry run: This issue passed the CQ dry run.

[JF, 2016-04-05 17:10:42]

The CQ bit was checked by jfb@chromium.org

[JF, 2016-04-05 17:10:42]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1837563002/#ps40001
(title: "Fix __has_feature")

[commit-bot: I haz the power, 2016-04-05 17:11:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1837563002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1837563002/40001

[Nico, 2016-04-05 18:43:38]

I'm very sorry about the huge amounts of back and forth for such a tiny change
:-(

https://codereview.chromium.org/1837563002/diff/40001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/1837563002/diff/40001/base/compiler_specific....
base/compiler_specific.h:193: #define __has_feature(FEATURE) 0
Hm, names starting with __ are reserved for the implementation. Redefining them
isn't valid. Do

#define COMPILER_HAS_FEATURE(x) 0
#else
#define COMPILER_HAS_FEATURE(x) __has_feature(x)

instead. (https://github.com/uclouvain/openjpeg/issues/727#issue-141639621 for a
similar recent example where someone defining something with __ caused issues.)

[commit-bot: I haz the power, 2016-04-05 20:52:12]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-05 20:53:24]

Description was changed from

==========
bit_cast: check is_trivially_copyable

Instead of using a misleading comment about POD-ness.

R=thakis@chromium.org
==========

to

==========
bit_cast: check is_trivially_copyable

Instead of using a misleading comment about POD-ness.

R=thakis@chromium.org

Committed: https://crrev.com/d81c1ced74e63e4bf50019ffd2d398def71bdae7
Cr-Commit-Position: refs/heads/master@{#385281}
==========

[commit-bot: I haz the power, 2016-04-05 20:53:25]

Patchset 3 (id:??) landed as
https://crrev.com/d81c1ced74e63e4bf50019ffd2d398def71bdae7
Cr-Commit-Position: refs/heads/master@{#385281}

[Mostyn Bramley-Moore, 2016-04-05 21:44:43]

@Nico: it seems that this breaks gcc builds, because std::is_trivially_copyable
is not available.  Investigating workarounds...

FAILED: ccache g++ -MMD -MF obj/net/filter/net.brotli_filter.o.d
-DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -D_FILE_OFFSET_BITS=64 -DDISABLE_NACL
-DCHROMIUM_BUILD -DCOMPONENT_BUILD -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1
-DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1
-DUSE_X11=1 -DUSE_CLIPBOARD_AURAX11=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1
-DENABLE_PEPPER_CDMS -DENABLE_NOTIFICATIONS -DENABLE_TOPCHROME_MD=1
-DDCHECK_ALWAYS_ON=1 -DFIELDTRIAL_TESTING_ENABLED -DENABLE_TASK_MANAGER=1
-DENABLE_EXTENSIONS=1 -DENABLE_PDF=1 -DENABLE_PLUGINS=1
-DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1
-DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1
-DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1
-DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_MDNS=1
-DENABLE_SERVICE_DISCOVERY=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING
-DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DNET_IMPLEMENTATION -DUSE_KERBEROS
-DDLOPEN_KERBEROS -DENABLE_BUILT_IN_DNS -DU_USING_ICU_NAMESPACE=0
-DU_ENABLE_DYLOAD=0 -DU_NOEXCEPT= -DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI
-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DBORINGSSL_SHARED_LIBRARY
-DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1
-DUSE_NSS_VERIFIER=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG
-DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen
-I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
-I../../third_party/protobuf -I../../third_party/protobuf/src -I../..
-Igen/protoc_out -I../../sdch/open-vcdiff/src -I../../third_party/zlib
-I../../third_party/boringssl/src/include -fstack-protector
--param=ssp-buffer-size=4 -Werror -pthread -fno-strict-aliasing -Wall -Wno-extra
-Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe
-fPIC -Wno-unused-local-typedefs -pthread -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/nss
-I/usr/include/nspr -m64 -march=x86-64 -O2 -fno-ident -fdata-sections
-ffunction-sections -funwind-tables -fno-exceptions -fno-rtti
-fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -Wno-narrowing 
-c ../../net/filter/brotli_filter.cc -o obj/net/filter/net.brotli_filter.o
In file included from ../../net/filter/brotli_filter.cc:7:0:
../../base/bit_cast.h: In function 'Dest bit_cast(const Source&)':
../../base/bit_cast.h:71:17: error: 'is_trivially_copyable' is not a member of
'std'
../../base/bit_cast.h:71:48: error: expected primary-expression before '>' token
../../base/bit_cast.h:71:49: error: '::value' has not been declared
../../base/bit_cast.h:73:17: error: 'is_trivially_copyable' is not a member of
'std'
../../base/bit_cast.h:73:50: error: expected primary-expression before '>' token
../../base/bit_cast.h:73:51: error: '::value' has not been declared

[JF, 2016-04-05 21:49:31]

On 2016/04/05 18:43:38, Nico (hiding until Fri) wrote:
> I'm very sorry about the huge amounts of back and forth for such a tiny change
> :-(
> 
> https://codereview.chromium.org/1837563002/diff/40001/base/compiler_specific.h
> File base/compiler_specific.h (right):
> 
>
https://codereview.chromium.org/1837563002/diff/40001/base/compiler_specific....
> base/compiler_specific.h:193: #define __has_feature(FEATURE) 0
> Hm, names starting with __ are reserved for the implementation. Redefining
them
> isn't valid. Do
> 
> #define COMPILER_HAS_FEATURE(x) 0
> #else
> #define COMPILER_HAS_FEATURE(x) __has_feature(x)
> 
> instead. (https://github.com/uclouvain/openjpeg/issues/727#issue-141639621 for
a
> similar recent example where someone defining something with __ caused
issues.)

Heh, no worries, this is super low priority :-)

I fixed in: https://codereview.chromium.org/1866473002
IIUC my fix was technically correct, but consistency is good anyways.

[JF, 2016-04-05 21:52:16]

On 2016/04/05 21:44:43, Mostyn Bramley-Moore wrote:
> @Nico: it seems that this breaks gcc builds, because
std::is_trivially_copyable
> is not available.  Investigating workarounds...

Which version of GCC and which version of libstdc++?

Do we have that version on any of the bots?

[JF, 2016-04-05 22:22:32]

On 2016/04/05 21:52:16, JF wrote:
> On 2016/04/05 21:44:43, Mostyn Bramley-Moore wrote:
> > @Nico: it seems that this breaks gcc builds, because
> std::is_trivially_copyable
> > is not available.  Investigating workarounds...
> 
> Which version of GCC and which version of libstdc++?
> 
> Do we have that version on any of the bots?

Moving discussion to: https://codereview.chromium.org/1863523005/

[Mostyn Bramley-Moore, 2016-04-05 23:10:27]

Description was changed from

==========
bit_cast: check is_trivially_copyable

Instead of using a misleading comment about POD-ness.

R=thakis@chromium.org

Committed: https://crrev.com/d81c1ced74e63e4bf50019ffd2d398def71bdae7
Cr-Commit-Position: refs/heads/master@{#385281}
==========

to

==========
bit_cast: check is_trivially_copyable

Instead of using a misleading comment about POD-ness.

R=thakis@chromium.org

Committed: https://crrev.com/d81c1ced74e63e4bf50019ffd2d398def71bdae7
Cr-Commit-Position: refs/heads/master@{#385281}
==========