Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2708593004:
Define illegal zone_allocator constructor only for MSVS (Closed)
|
Created:
3 years, 10 months ago by Clemens Hammacher Modified:
3 years, 10 months ago Reviewers:
bbudge CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionDefine illegal zone_allocator constructor only for MSVS
It turns out that the default constructor of allocators used in
standard containers is still needed in MSVS 2015.
This CL defines the constructor only when compiling on windows.
R=bbudge@chromium.org
Review-Url: https://codereview.chromium.org/2708593004
Cr-Commit-Position: refs/heads/master@{#43341}
Committed: https://chromium.googlesource.com/v8/v8/+/a182f8d510983e22cc05e16ae4ce7c43a6ecb07d
Patch Set 1 #Patch Set 2 : Add ifdef to still define the constructor on windows #
Total comments: 2
Patch Set 3 : Use V8_CC_MSVC instead of V8_OS_WIN #Messages
Total messages: 23 (17 generated)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove obsolete and illegal zone_allocator constructor Now that we switched to MSVS 2015, the constructor can finally be removed. R=bbudge@chromium.org ========== to ========== Define illegal zone_allocator constructor only for MSVS It turns out that the default constructor of allocators used in standard containers is still needed in MSVS 2015. This CL defines the constructor only when compiling on windows. R=bbudge@chromium.org ==========
I ran into this UNREACHABLE, so I tried removing the obsolete constructor. Unfortunately, MSVS 2015 still seems to need it: https://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/... So this CL now at least wraps it in an ifdef to only define it on windows.
lgtm
One comment. https://codereview.chromium.org/2708593004/diff/20001/src/zone/zone-allocator.h File src/zone/zone-allocator.h (right): https://codereview.chromium.org/2708593004/diff/20001/src/zone/zone-allocator... src/zone/zone-allocator.h:29: #ifdef V8_OS_WIN Perhaps V8_CC_MSVC would be a better filter.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2708593004/diff/20001/src/zone/zone-allocator.h File src/zone/zone-allocator.h (right): https://codereview.chromium.org/2708593004/diff/20001/src/zone/zone-allocator... src/zone/zone-allocator.h:29: #ifdef V8_OS_WIN On 2017/02/20 at 19:43:05, bbudge wrote: > Perhaps V8_CC_MSVC would be a better filter. Good proposal, thanks. Changed it accordingly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/2708593004/#ps40001 (title: "Use V8_CC_MSVC instead of V8_OS_WIN")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487672118869250,
"parent_rev": "b0c7a0fc6d0417eee2e55a392133bd2e88341cb0", "commit_rev":
"a182f8d510983e22cc05e16ae4ce7c43a6ecb07d"}
Message was sent while issue was closed.
Description was changed from ========== Define illegal zone_allocator constructor only for MSVS It turns out that the default constructor of allocators used in standard containers is still needed in MSVS 2015. This CL defines the constructor only when compiling on windows. R=bbudge@chromium.org ========== to ========== Define illegal zone_allocator constructor only for MSVS It turns out that the default constructor of allocators used in standard containers is still needed in MSVS 2015. This CL defines the constructor only when compiling on windows. R=bbudge@chromium.org Review-Url: https://codereview.chromium.org/2708593004 Cr-Commit-Position: refs/heads/master@{#43341} Committed: https://chromium.googlesource.com/v8/v8/+/a182f8d510983e22cc05e16ae4ce7c43a6e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/a182f8d510983e22cc05e16ae4ce7c43a6e... |
