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

Issue 399313006: Move bit_cast from base/macros.h to its own header (Closed)

Created:
5 years, 3 months ago by tapted
Modified:
3 years, 9 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, chromoting-reviews_chromium.org, jam, darin-cc_chromium.org, piman+watch_chromium.org, erikwright+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, viettrungluu
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 : 2014 flavour #

Patch Set 2 : Rebase (string.h still in macros.h) #

Patch Set 3 : Zap the string.h include from macros.h [iwyu pass1] #

Patch Set 4 : pass5 #

Patch Set 5 : finally? #

Total comments: 12

Patch Set 6 : bit_cast spruce up #

Patch Set 7 : bit_cast spruce up+fix typo #

Total comments: 2

Patch Set 8 : rebase: brotli_filter.cc #

Patch Set 9 : guard asserts #

Patch Set 10 : remove asserts, <type_traits>, restore warning #

Patch Set 11 : Update crashpad after making http://crrev.com/1555943002 a dependent CL #

Total comments: 2

Patch Set 12 : Rebase for r367322 #

Patch Set 13 : respond to comments #

Patch Set 14 : Rebase for crashpad roll (r367899) #

Total comments: 2

Patch Set 15 : rebase (only) #

Patch Set 16 : Fix chromeos compile after r368203 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -67 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M base/base.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
A base/bit_cast.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +71 lines, -0 lines 0 comments Download
M base/macros.h View 1 2 2 chunks +0 lines, -65 lines 0 comments Download
M base/time/time_win.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/profile_resetter/triggered_profile_resetter_win_unittest.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M chromeos/hugepage_text/hugepage_text.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M components/compression/compression_utils.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/visitedlink/common/visitedlink_common.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/webcrypto/algorithms/rsa_oaep.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/pepper/pepper_plugin_instance_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M courgette/difference_estimator.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M gpu/command_buffer/tests/gl_readback_unittest.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M media/cdm/ppapi/ppapi_cdm_adapter.cc View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M net/disk_cache/blockfile/bitmap.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M net/disk_cache/blockfile/disk_format_v3.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M net/disk_cache/blockfile/index_table_v3.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M net/filter/brotli_filter.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M net/filter/gzip_filter.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M net/filter/gzip_filter_unittest.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M net/filter/sdch_filter_unittest.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M net/quic/test_tools/mock_random.cc View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M pdf/out_of_process_instance.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M ppapi/shared_impl/ppb_view_shared.cc View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M remoting/client/plugin/pepper_util.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M sandbox/win/src/Wow64.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M sandbox/win/src/service_resolver_32.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M sandbox/win/src/service_resolver_unittest.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M sandbox/win/wow_helper/service64_resolver.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M sync/internal_api/http_bridge.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M sync/internal_api/http_bridge_unittest.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M third_party/crashpad/README.chromium View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
M third_party/crashpad/crashpad/util/numeric/int128_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 59 (29 generated)
commit-bot: I haz the power
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
3 years, 9 months ago (2015-12-29 05:36:08 UTC) #6
commit-bot: I haz the power
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-generic_chromium_compile_only_ng/builds/74260)
3 years, 9 months ago (2015-12-29 05:50:15 UTC) #9
tapted
Hi Brett, please take a look (WDYT?)
3 years, 9 months ago (2015-12-30 11:57:58 UTC) #16
Avi (use Gerrit)
lgtm Don't hold up this move on my comments, but bit_cast could use a bit ...
3 years, 9 months ago (2015-12-30 16:57:37 UTC) #18
Avi (use Gerrit)
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 ...
3 years, 9 months ago (2015-12-30 17:32:43 UTC) #19
tapted
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 > ...
3 years, 9 months ago (2016-01-03 23:36:58 UTC) #20
tapted
also +viettrungluu for the updates in bit_cast.h, since I think you were the original mastermind ...
3 years, 9 months ago (2016-01-03 23:51:07 UTC) #23
Avi (use Gerrit)
Looking good! The next step is to move those static_asserts of trivial copyability back upstream ...
3 years, 9 months ago (2016-01-04 00:24:46 UTC) #24
tapted
(also I see http://crbug.com/572874 [bit_cast compile error in `crashpad_util_test`] is still not resolved - I'll ...
3 years, 9 months ago (2016-01-04 00:59:30 UTC) #25
Avi (use Gerrit)
It's probably best to put this off entirely to a different CL, and use this ...
3 years, 9 months ago (2016-01-04 01:32:19 UTC) #26
tapted
+mark for third_party/crashpad OWNERS On 2016/01/04 01:32:19, Avi wrote: > It's probably best to put ...
3 years, 9 months ago (2016-01-04 10:52:45 UTC) #30
Mark Mentovai
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 ...
3 years, 9 months ago (2016-01-04 14:44:35 UTC) #31
viettrungluu
On 2016/01/03 23:51:07, tapted wrote: > also +viettrungluu for the updates in bit_cast.h, since I ...
3 years, 9 months ago (2016-01-04 20:20:59 UTC) #32
Avi (use Gerrit)
On 2016/01/04 20:20:59, viettrungluu wrote: > On 2016/01/03 23:51:07, tapted wrote: > > also +viettrungluu ...
3 years, 9 months ago (2016-01-04 20:33:29 UTC) #33
brettw
It was copied from google3's basictypes.h, not in base/casts.h
3 years, 9 months ago (2016-01-04 21:48:54 UTC) #34
brettw
On 2016/01/04 21:48:54, brettw wrote: > It was copied from google3's basictypes.h, not in base/casts.h ...
3 years, 9 months ago (2016-01-04 21:49:09 UTC) #35
tapted
brettw: could you please review for src/OWNERS ? On 2016/01/04 21:48:54, brettw wrote: > It ...
3 years, 9 months ago (2016-01-04 23:20:30 UTC) #36
Mark Mentovai
brettw wrote: > On 2016/01/04 21:48:54, brettw wrote: > > It was copied from google3's ...
3 years, 9 months ago (2016-01-06 18:20:04 UTC) #38
Avi (use Gerrit)
On 2016/01/06 18:20:04, Mark Mentovai wrote: > brettw wrote: > > On 2016/01/04 21:48:54, brettw ...
3 years, 9 months ago (2016-01-06 18:39:40 UTC) #39
tapted
On 2016/01/06 18:39:40, Avi wrote: > On 2016/01/06 18:20:04, Mark Mentovai wrote: > > brettw ...
3 years, 9 months ago (2016-01-06 23:18:26 UTC) #40
tapted
brettw: ping?
3 years, 9 months ago (2016-01-12 22:55:10 UTC) #41
brettw
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 ...
3 years, 9 months ago (2016-01-13 19:16:24 UTC) #42
tapted
Thanks all! (and I'll probably need to rebase, but I might as well try my ...
3 years, 9 months ago (2016-01-13 21:59:27 UTC) #43
commit-bot: I haz the power
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
3 years, 9 months ago (2016-01-13 22:00:39 UTC) #46
commit-bot: I haz the power
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 ...
3 years, 9 months ago (2016-01-14 00:12:16 UTC) #48
commit-bot: I haz the power
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
3 years, 9 months ago (2016-01-14 02:12:42 UTC) #51
commit-bot: I haz the power
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 ...
3 years, 9 months ago (2016-01-14 04:18:55 UTC) #53
commit-bot: I haz the power
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
3 years, 9 months ago (2016-01-14 06:20:11 UTC) #55
commit-bot: I haz the power
Committed patchset #16 (id:500001)
3 years, 9 months ago (2016-01-14 06:26:46 UTC) #57
commit-bot: I haz the power
3 years, 9 months ago (2016-01-14 06:28:22 UTC) #59
Message was sent while issue was closed.
Patchset 16 (id:??) landed as
https://crrev.com/174fde33258d2128a25982fbaded0ffeefa2cd6d
Cr-Commit-Position: refs/heads/master@{#369348}

Powered by Google App Engine
This is Rietveld 408576698