Move bit_cast from base/macros.h to its own header

Move bit_cast from base/macros.h to its own header

There isn't much that uses bit_cast. bit_cast requires memcpy which
requires <string.h>

On Mac, <string.h> includes AvailabilityInternal.h which is half a
megabyte and defines dozens of macros. Providing <string.h> in
base/macros.h also tempts IWYU violations. Try to avoid it.

Add base/bit_cast.h and include it from the 17 files that need it. Then
remove the <string.h> #include from base/macros.h and IWYU.

BUG=395001
Committed: https://crrev.com/174fde33258d2128a25982fbaded0ffeefa2cd6d
Cr-Commit-Position: refs/heads/master@{#369348}

[tapted, 2015-12-29 05:06:18]

Patchset #3 (id:60001) has been deleted

[tapted, 2015-12-29 05:06:26]

Patchset #2 (id:20001) has been deleted

[tapted, 2015-12-29 05:06:38]

Patchset #1 (id:1) has been deleted

[tapted, 2015-12-29 05:07:29]

Patchset #2 (id:100001) has been deleted

[tapted, 2015-12-29 05:35:53]

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

[commit-bot: I haz the power, 2015-12-29 05:36:08]

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

[tapted, 2015-12-29 05:37:41]

Description was changed from

==========
Move bit_cast from base/macros.h to its own header

There isn't much that uses bit_cast. bit_cast requires memcpy which
requires string.h

On Mac, string.h includes AvailabilityInternal.h which is half a
megabyte and defines dozens of macros. Try to avoid it.

This first stage moves bit_cast out of macros.h but leaves the string.h
#include in base/basictypes.h to remove in follow-up CLs.

BUG=395001
==========

to

==========
Move bit_cast from base/macros.h to its own header

There isn't much that uses bit_cast. bit_cast requires memcpy which
requires string.h

On Mac, string.h includes AvailabilityInternal.h which is half a
megabyte and defines dozens of macros. Try to avoid it.

Add base/bit_cast.h and include it from the 17 files that need it. Then
remove the string.h #include from base/macros.h and IWYU.

BUG=395001
==========

[commit-bot: I haz the power, 2015-12-29 05:50:14]

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

[commit-bot: I haz the power, 2015-12-29 05:50:15]

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...)

[tapted, 2015-12-30 04:30:58]

Description was changed from

==========
Move bit_cast from base/macros.h to its own header

There isn't much that uses bit_cast. bit_cast requires memcpy which
requires string.h

On Mac, string.h includes AvailabilityInternal.h which is half a
megabyte and defines dozens of macros. Try to avoid it.

Add base/bit_cast.h and include it from the 17 files that need it. Then
remove the string.h #include from base/macros.h and IWYU.

BUG=395001
==========

to

==========
Move bit_cast from base/macros.h to its own header

There isn't much that uses bit_cast. bit_cast requires memcpy which
requires string.h

On Mac, string.h includes AvailabilityInternal.h which is half a
megabyte and defines dozens of macros. Providing string.h in
base/macros.h also tempts IWYU violations. Try to avoid it.

Add base/bit_cast.h and include it from the 17 files that need it. Then
remove the string.h #include from base/macros.h and IWYU.

BUG=395001
==========

[tapted, 2015-12-30 06:51:14]

Patchset #6 (id:200001) has been deleted

[tapted, 2015-12-30 06:51:29]

Patchset #5 (id:180001) has been deleted

[tapted, 2015-12-30 06:51:59]

Patchset #4 (id:160001) has been deleted

[tapted, 2015-12-30 06:52:56]

Description was changed from

==========
Move bit_cast from base/macros.h to its own header

There isn't much that uses bit_cast. bit_cast requires memcpy which
requires string.h

On Mac, string.h includes AvailabilityInternal.h which is half a
megabyte and defines dozens of macros. Providing string.h in
base/macros.h also tempts IWYU violations. Try to avoid it.

Add base/bit_cast.h and include it from the 17 files that need it. Then
remove the string.h #include from base/macros.h and IWYU.

BUG=395001
==========

to

==========
Move bit_cast from base/macros.h to its own header

There isn't much that uses bit_cast. bit_cast requires memcpy which
requires <string.h>

On Mac, <string.h> includes AvailabilityInternal.h which is half a
megabyte and defines dozens of macros. Providing <string.h> in
base/macros.h also tempts IWYU violations. Try to avoid it.

Add base/bit_cast.h and include it from the 17 files that need it. Then
remove the <string.h> #include from base/macros.h and IWYU.

BUG=395001
==========

[tapted, 2015-12-30 11:57:57]

tapted@chromium.org changed reviewers:
+ brettw@chromium.org

[tapted, 2015-12-30 11:57:58]

Hi Brett, please take a look (WDYT?)

[Avi (use Gerrit), 2015-12-30 16:57:37]

avi@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2015-12-30 16:57:37]

lgtm

Don't hold up this move on my comments, but bit_cast could use a bit of cleanup
too :)

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode26
base/bit_cast.h:26: // according to ISO C++ specification section 3.10 -15 -. 
Roughly, this
"-15 -"?

Perhaps:

"according to the ISO C++98 specification, section 3.10 ("basic.lval"),
paragraph 15. (This did not substantially change in C++11.)"

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode35
base/bit_cast.h:35: // The purpose of 3.10 -15- is to allow optimizing compilers
to assume
"-15-" again?

"The purpose of this paragraph..."

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode37
base/bit_cast.h:37: // 4.0.1 has an optimizer that takes advantage of this.  So
a
GCC 4.0.1 was released in 2005. Do we want to keep this reference or just
vagueify the paragraph a bit as "this is the right thing to do"?

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode59
base/bit_cast.h:59: // I tested this code with gcc 2.95.3, gcc 4.0.1, icc 8.1,
and msvc 7.1.
More references to incredibly obsolete compilers :(

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode62
base/bit_cast.h:62: // is likely to surprise you.
We should totally rewrite this template using the std::is_pod type trait so it
won't compile in this case.

[Avi (use Gerrit), 2015-12-30 17:32:43]

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode67
base/bit_cast.h:67: "bit_cast requires source and destination to be the same
size");
Actually, just insert two lines:

static_assert(std::is_pod<Source>::value, "Source must be a POD type");
static_assert(std::is_pod<Dest>::value, "Source must be a POD type");

with

#include <type_traits>

[tapted, 2016-01-03 23:36:58]

On 2015/12/30 17:32:43, Avi wrote:
> https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h
> File base/bit_cast.h (right):
> 
>
https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode67
> base/bit_cast.h:67: "bit_cast requires source and destination to be the same
> size");
> Actually, just insert two lines:
> 
> static_assert(std::is_pod<Source>::value, "Source must be a POD type");
> static_assert(std::is_pod<Dest>::value, "Source must be a POD type");
> 
> with
> 
> #include <type_traits>

I went with is_trivially_copyable, since there's an example that fails with
is_pod.

../../base/bit_cast.h:66:3: error: static_assert failed "Source must be a POD
type"
  static_assert(std::is_pod<Source>::value, "Source must be a POD type");
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~
../../content/renderer/pepper/pepper_plugin_instance_impl.cc:455:28: note: in
instantiation of function template specialization
'bit_cast<ppapi::WebKitGamepads, blink::WebGamepads>' requested here
  ConvertWebKitGamepadData(bit_cast<ppapi::WebKitGamepads>(webkit_data), data);


The affected type has user-supplied constructors, which aren't allowed with
is_pod. is_trivially_copyable is a good fit though -
http://en.cppreference.com/w/cpp/types/is_trivially_copyable

[tapted, 2016-01-03 23:44:20]

Patchset #6 (id:260001) has been deleted

[tapted, 2016-01-03 23:51:05]

tapted@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[tapted, 2016-01-03 23:51:07]

also +viettrungluu for the updates in bit_cast.h, since I think you were the
original mastermind for this

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode26
base/bit_cast.h:26: // according to ISO C++ specification section 3.10 -15 -. 
Roughly, this
On 2015/12/30 16:57:37, Avi wrote:
> "-15 -"?
> 
> Perhaps:
> 
> "according to the ISO C++98 specification, section 3.10 ("basic.lval"),
> paragraph 15. (This did not substantially change in C++11.)"

Done.

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode35
base/bit_cast.h:35: // The purpose of 3.10 -15- is to allow optimizing compilers
to assume
On 2015/12/30 16:57:37, Avi wrote:
> "-15-" again?
> 
> "The purpose of this paragraph..."

Done.

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode37
base/bit_cast.h:37: // 4.0.1 has an optimizer that takes advantage of this.  So
a
On 2015/12/30 16:57:37, Avi wrote:
> GCC 4.0.1 was released in 2005. Do we want to keep this reference or just
> vagueify the paragraph a bit as "this is the right thing to do"?

I went with "Compilers are known to take advantage of this." (i.e. to keep the
message that bit_cast exists because of known problems rather than merely an
interpretation of the standard)

aside: Interestingly... Chrome compiles with -fno-strict-aliasing which turns
off these kinds of optimizations (as well as turning off the warnings that
dereferencing a "type punned" variable can lead to bad codegen when the compiler
assumes it isn't creating an alias for data of a different type...).

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode59
base/bit_cast.h:59: // I tested this code with gcc 2.95.3, gcc 4.0.1, icc 8.1,
and msvc 7.1.
On 2015/12/30 16:57:37, Avi wrote:
> More references to incredibly obsolete compilers :(

I think what's happening anyway is not that memcpy is fast, but that compilers
can replace calls to the memcpy function with inline code when the `size`
argument is a compile-time constant....  (and compilers can't make aliasing
assumptions after passing an address to a function, so there's effectively an
optimization barrier at every bit_cast).

Replaced this and the above para with

// Fortunately memcpy() is very fast.  In optimized mode, compilers replace
// calls to memcpy() with inline object code when the size argument is a
// compile-time constant.  On a 32-bit system, memcpy(d,s,4) compiles to one
// load and one store, and memcpy(d,s,8) compiles to two loads and two stores.

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode62
base/bit_cast.h:62: // is likely to surprise you.
On 2015/12/30 16:57:37, Avi wrote:
> We should totally rewrite this template using the std::is_pod type trait so it
> won't compile in this case.

Done.

https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode67
base/bit_cast.h:67: "bit_cast requires source and destination to be the same
size");
On 2015/12/30 17:32:43, Avi wrote:
> Actually, just insert two lines:
> 
> static_assert(std::is_pod<Source>::value, "Source must be a POD type");
> static_assert(std::is_pod<Dest>::value, "Source must be a POD type");
> 
> with
> 
> #include <type_traits>

Done.

[Avi (use Gerrit), 2016-01-04 00:24:46]

Looking good! The next step is to move those static_asserts of trivial
copyability back upstream to google3 and see what explodes :)

LGTM

https://codereview.chromium.org/399313006/diff/300001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/399313006/diff/300001/base/bit_cast.h#newcode69
base/bit_cast.h:69: "Dest must be a POD type or trivially copyable");
This is indeed a better choice than is_pod.

[tapted, 2016-01-04 00:59:30]

(also I see http://crbug.com/572874 [bit_cast compile error in
`crashpad_util_test`] is still not resolved - I'll need to ensure that's happy
before landing this -- sorry for the noise!)

https://codereview.chromium.org/399313006/diff/300001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/399313006/diff/300001/base/bit_cast.h#newcode69
base/bit_cast.h:69: "Dest must be a POD type or trivially copyable");
On 2016/01/04 00:24:46, Avi wrote:
> This is indeed a better choice than is_pod.

Doh - bots are still reporting in, but it looks like there was some lag in
compilers/libraries providing is_trivially_copyable. gcc only provides it in
libstdc++ since version 5. But some failures are on clang -- it looks like there
are bot configurations compiling with clang, but using gcc's libstdc++ from 4.x
(which surprises me - I'll try to figure out whether this is a proper
configuration).

There might be a workaround like

#if (__has_feature(is_trivially_copyable) && defined(_LIBCPP_VERSION)) ||      \
    (defined(__GNUC__) && __GNUC__ >= 5)
#define IS_TRIVIALLY_COPYABLE(T) std::is_trivially_copyable<T>::value;
#elif __has_feature(is_trivially_copyable)
#define IS_TRIVIALLY_COPYABLE(T) __is_trivially_copyable(T);
#else
#error "Can't define IS_TRIVIALLY_COPYABLE"
#endif

But I need to deploy some test CLs. Also this looks a little gross -- it might
be better to put off adding the static_assert for now, or just guarding the
asserts with _LIBCPP_VERSION, since it's likely at least one of the bots will be
compiling with libc++, so they will still be effective in picking up mistakes.

[Avi (use Gerrit), 2016-01-04 01:32:19]

It's probably best to put this off entirely to a different CL, and use this one
just to get bit_cast into its own header.

[tapted, 2016-01-04 03:41:34]

Patchset #10 (id:320002) has been deleted

[tapted, 2016-01-04 04:44:43]

Patchset #11 (id:380001) has been deleted

[tapted, 2016-01-04 10:52:44]

tapted@chromium.org changed reviewers:
+ mark@chromium.org

[tapted, 2016-01-04 10:52:45]

+mark for third_party/crashpad OWNERS

On 2016/01/04 01:32:19, Avi wrote:
> It's probably best to put this off entirely to a different CL, and use this
one
> just to get bit_cast into its own header.

Done. (removed the extra static_asserts/type_traits and restored the warning).

There's a working fix with green runs in Patch Set 9 (diff:
https://codereview.chromium.org/399313006/diff2/320001:340001/base/bit_cast.h ).
It turns out MSVC also has a buggy implementation of is_trivially_copyable that
caused a hiccup :/. See
https://connect.microsoft.com/VisualStudio/feedback/details/806233 . There's an
MSFT update saying "fixed" (date: Aug 2014), but it doesn't look like Chrome is
on that yet.

On 2016/01/04 00:59:30, tapted wrote:
> (also I see http://crbug.com/572874 [bit_cast compile error in
> `crashpad_util_test`] is still not resolved - I'll need to ensure that's happy
> before landing this

fix for this is now in a depends patchset (to land before this CL). However,
this CL would break things again without another update to third_party/crashpad,
so that needs to be rolled in.

[Mark Mentovai, 2016-01-04 14:44:35]

LGTM in third_party/crashpad

https://codereview.chromium.org/399313006/diff/400001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/399313006/diff/400001/base/bit_cast.h#newcode8
base/bit_cast.h:8: #include <string.h>  // For memcpy.
We don’t usually need to say why we’re including headers, and this one is pretty
obvious because the whole file only uses one library function.

[viettrungluu, 2016-01-04 20:20:59]

On 2016/01/03 23:51:07, tapted wrote:
> also +viettrungluu for the updates in bit_cast.h, since I think you were the
> original mastermind for this

It pre-dates me, I think. Not sure who wrote it, but probably someone who knows
more C++ than me. :)

> 
> https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h
> File base/bit_cast.h (right):
> 
>
https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode26
> base/bit_cast.h:26: // according to ISO C++ specification section 3.10 -15 -. 
> Roughly, this
> On 2015/12/30 16:57:37, Avi wrote:
> > "-15 -"?
> > 
> > Perhaps:
> > 
> > "according to the ISO C++98 specification, section 3.10 ("basic.lval"),
> > paragraph 15. (This did not substantially change in C++11.)"
> 
> Done.
> 
>
https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode35
> base/bit_cast.h:35: // The purpose of 3.10 -15- is to allow optimizing
compilers
> to assume
> On 2015/12/30 16:57:37, Avi wrote:
> > "-15-" again?
> > 
> > "The purpose of this paragraph..."
> 
> Done.
> 
>
https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode37
> base/bit_cast.h:37: // 4.0.1 has an optimizer that takes advantage of this. 
So
> a
> On 2015/12/30 16:57:37, Avi wrote:
> > GCC 4.0.1 was released in 2005. Do we want to keep this reference or just
> > vagueify the paragraph a bit as "this is the right thing to do"?
> 
> I went with "Compilers are known to take advantage of this." (i.e. to keep the
> message that bit_cast exists because of known problems rather than merely an
> interpretation of the standard)
> 
> aside: Interestingly... Chrome compiles with -fno-strict-aliasing which turns
> off these kinds of optimizations (as well as turning off the warnings that
> dereferencing a "type punned" variable can lead to bad codegen when the
compiler
> assumes it isn't creating an alias for data of a different type...).
> 
>
https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode59
> base/bit_cast.h:59: // I tested this code with gcc 2.95.3, gcc 4.0.1, icc 8.1,
> and msvc 7.1.
> On 2015/12/30 16:57:37, Avi wrote:
> > More references to incredibly obsolete compilers :(
> 
> I think what's happening anyway is not that memcpy is fast, but that compilers
> can replace calls to the memcpy function with inline code when the `size`
> argument is a compile-time constant....  (and compilers can't make aliasing
> assumptions after passing an address to a function, so there's effectively an
> optimization barrier at every bit_cast).
> 
> Replaced this and the above para with
> 
> // Fortunately memcpy() is very fast.  In optimized mode, compilers replace
> // calls to memcpy() with inline object code when the size argument is a
> // compile-time constant.  On a 32-bit system, memcpy(d,s,4) compiles to one
> // load and one store, and memcpy(d,s,8) compiles to two loads and two stores.
> 
>
https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode62
> base/bit_cast.h:62: // is likely to surprise you.
> On 2015/12/30 16:57:37, Avi wrote:
> > We should totally rewrite this template using the std::is_pod type trait so
it
> > won't compile in this case.
> 
> Done.
> 
>
https://codereview.chromium.org/399313006/diff/240001/base/bit_cast.h#newcode67
> base/bit_cast.h:67: "bit_cast requires source and destination to be the same
> size");
> On 2015/12/30 17:32:43, Avi wrote:
> > Actually, just insert two lines:
> > 
> > static_assert(std::is_pod<Source>::value, "Source must be a POD type");
> > static_assert(std::is_pod<Dest>::value, "Source must be a POD type");
> > 
> > with
> > 
> > #include <type_traits>
> 
> Done.

[Avi (use Gerrit), 2016-01-04 20:33:29]

On 2016/01/04 20:20:59, viettrungluu wrote:
> On 2016/01/03 23:51:07, tapted wrote:
> > also +viettrungluu for the updates in bit_cast.h, since I think you were the
> > original mastermind for this
> 
> It pre-dates me, I think. Not sure who wrote it, but probably someone who
knows
> more C++ than me. :)

bit_cast was added in cl/1678788 by mec@ in 2005, and found its way into our
repo before initial.commit.
(https://chromium.googlesource.com/chromium/src/+/d7cae12696b96500c05dd2d430f6...).

[brettw, 2016-01-04 21:48:54]

It was copied from google3's basictypes.h, not in base/casts.h

[brettw, 2016-01-04 21:49:09]

On 2016/01/04 21:48:54, brettw wrote:
> It was copied from google3's basictypes.h, not in base/casts.h

That should read "now in".

[tapted, 2016-01-04 23:20:30]

brettw: could you please review for src/OWNERS ?

On 2016/01/04 21:48:54, brettw wrote:
> It was copied from google3's basictypes.h, now in base/casts.h

Ah, that explains it. I guess that's more justification to do move bit_cast
somewhere since it's something of an orphan :).

Maybe google3 compilers will fare better with std::is_trivially_copyable. The
things it picked up in Chrome were just compiler/library bugs. (we'd need to
drop support for gcc <5.0 and advance our win toolchain to avoid #ifdefs).

https://codereview.chromium.org/399313006/diff/400001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/399313006/diff/400001/base/bit_cast.h#newcode8
base/bit_cast.h:8: #include <string.h>  // For memcpy.
On 2016/01/04 14:44:34, Mark Mentovai wrote:
> We don’t usually need to say why we’re including headers, and this one is
pretty
> obvious because the whole file only uses one library function.

Done.

[tapted, 2016-01-04 23:21:07]

tapted@chromium.org changed reviewers:
- viettrungluu@chromium.org

[Mark Mentovai, 2016-01-06 18:20:04]

brettw wrote:
> On 2016/01/04 21:48:54, brettw wrote:
> > It was copied from google3's basictypes.h, not in base/casts.h
> 
> That should read "now in".

If google3 calls it base/casts.h, maybe we should too.

[Avi (use Gerrit), 2016-01-06 18:39:40]

On 2016/01/06 18:20:04, Mark Mentovai wrote:
> brettw wrote:
> > On 2016/01/04 21:48:54, brettw wrote:
> > > It was copied from google3's basictypes.h, not in base/casts.h
> > 
> > That should read "now in".
> 
> If google3 calls it base/casts.h, maybe we should too.

google3 has multiple casts in base/casts.h. We have but one, and I doubt we'll
add any of the others.

[tapted, 2016-01-06 23:18:26]

On 2016/01/06 18:39:40, Avi wrote:
> On 2016/01/06 18:20:04, Mark Mentovai wrote:
> > brettw wrote:
> > > On 2016/01/04 21:48:54, brettw wrote:
> > > > It was copied from google3's basictypes.h, not in base/casts.h
> > > 
> > > That should read "now in".
> > 
> > If google3 calls it base/casts.h, maybe we should too.
> 
> google3 has multiple casts in base/casts.h. We have but one, and I doubt we'll
> add any of the others.

I'm on the side of bit_cast.h, but I'm happy to be overruled :). One reason I
can give is that it's unique amongst the casts in requiring <string.h> -- and
avoiding the pollution of <string.h> through all of chrome is one of the goals
of this CL. While that's equally served with a different name, bundling it with
other casts could regress that in future.

Auditing the other casts, we once had implicit_cast in macros.h, but it was
misused and didn't add much so it's since been purged from Chrome. Another cast
in base/casts.h requires RTTI in debug mode. The last relates to enums - it's
interesting, but it's a 50-line probably-optimized-away inline function and more
(including <limits.h>) -- I think it would be well-suited to having its own
header as well.

[tapted, 2016-01-12 22:55:10]

brettw: ping?

[brettw, 2016-01-13 19:16:24]

LGTM

https://codereview.chromium.org/399313006/diff/460001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/399313006/diff/460001/base/bit_cast.h#newcode62
base/bit_cast.h:62: inline Dest bit_cast(const Source& source) {
We can do this in a follow-up if you prefer, but this should really be in the
base namespace. The macros header is kind of a mess and it looks like only some
of the stuff is in the right namespace.

[tapted, 2016-01-13 21:59:27]

Thanks all!

(and I'll probably need to rebase, but I might as well try my luck..)

https://codereview.chromium.org/399313006/diff/460001/base/bit_cast.h
File base/bit_cast.h (right):

https://codereview.chromium.org/399313006/diff/460001/base/bit_cast.h#newcode62
base/bit_cast.h:62: inline Dest bit_cast(const Source& source) {
On 2016/01/13 19:16:23, brettw wrote:
> We can do this in a follow-up if you prefer, but this should really be in the
> base namespace. The macros header is kind of a mess and it looks like only
some
> of the stuff is in the right namespace.

Yeah I considered moving it to base:: at the same time early on... I'll make a
follow-up CL.

[tapted, 2016-01-13 21:59:38]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-01-13 21:59:39]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
mark@chromium.org
Link to the patchset: https://codereview.chromium.org/399313006/#ps460001
(title: "Rebase for crashpad roll (r367899)")

[commit-bot: I haz the power, 2016-01-13 22:00:39]

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

[commit-bot: I haz the power, 2016-01-14 00:12:15]

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

[commit-bot: I haz the power, 2016-01-14 00:12:16]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[tapted, 2016-01-14 02:02:57]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-01-14 02:02:57]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
avi@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/399313006/#ps500001
(title: "Fix chromeos compile after r368203")

[commit-bot: I haz the power, 2016-01-14 02:12:42]

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

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

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

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

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)

[tapted, 2016-01-14 06:19:53]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-01-14 06:20:11]

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

[commit-bot: I haz the power, 2016-01-14 06:26:44]

Description was changed from

==========
Move bit_cast from base/macros.h to its own header

There isn't much that uses bit_cast. bit_cast requires memcpy which
requires <string.h>

On Mac, <string.h> includes AvailabilityInternal.h which is half a
megabyte and defines dozens of macros. Providing <string.h> in
base/macros.h also tempts IWYU violations. Try to avoid it.

Add base/bit_cast.h and include it from the 17 files that need it. Then
remove the <string.h> #include from base/macros.h and IWYU.

BUG=395001
==========

to

==========
Move bit_cast from base/macros.h to its own header

There isn't much that uses bit_cast. bit_cast requires memcpy which
requires <string.h>

On Mac, <string.h> includes AvailabilityInternal.h which is half a
megabyte and defines dozens of macros. Providing <string.h> in
base/macros.h also tempts IWYU violations. Try to avoid it.

Add base/bit_cast.h and include it from the 17 files that need it. Then
remove the <string.h> #include from base/macros.h and IWYU.

BUG=395001
==========

[commit-bot: I haz the power, 2016-01-14 06:26:46]

Committed patchset #16 (id:500001)

[commit-bot: I haz the power, 2016-01-14 06:28:21]

Description was changed from

==========
Move bit_cast from base/macros.h to its own header

There isn't much that uses bit_cast. bit_cast requires memcpy which
requires <string.h>

On Mac, <string.h> includes AvailabilityInternal.h which is half a
megabyte and defines dozens of macros. Providing <string.h> in
base/macros.h also tempts IWYU violations. Try to avoid it.

Add base/bit_cast.h and include it from the 17 files that need it. Then
remove the <string.h> #include from base/macros.h and IWYU.

BUG=395001
==========

to

==========
Move bit_cast from base/macros.h to its own header

There isn't much that uses bit_cast. bit_cast requires memcpy which
requires <string.h>

On Mac, <string.h> includes AvailabilityInternal.h which is half a
megabyte and defines dozens of macros. Providing <string.h> in
base/macros.h also tempts IWYU violations. Try to avoid it.

Add base/bit_cast.h and include it from the 17 files that need it. Then
remove the <string.h> #include from base/macros.h and IWYU.

BUG=395001

Committed: https://crrev.com/174fde33258d2128a25982fbaded0ffeefa2cd6d
Cr-Commit-Position: refs/heads/master@{#369348}
==========

[commit-bot: I haz the power, 2016-01-14 06:28:22]

Patchset 16 (id:??) landed as
https://crrev.com/174fde33258d2128a25982fbaded0ffeefa2cd6d
Cr-Commit-Position: refs/heads/master@{#369348}