Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1673563002:
Replace base::Tuple implementation with std::tuple (Closed)
DescriptionReplace base::Tuple implementation with std::tuple
* Remove base::Tuple and make base::Tuple as an alias of std::tuple.
* Expand the alias where it's used in a class template specialization to avoid MSVC2013 internal compiler error.
BUG=554987
Committed: https://crrev.com/9ca3021996e87ad03adf4451fd7fc7f8097c8f46
Cr-Commit-Position: refs/heads/master@{#374878}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : rebase #Patch Set 8 : #
Messages
Total messages: 45 (19 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673563002/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673563002/50001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673563002/70001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673563002/70001
Description was changed from ========== Replace base::Tuple with std::tuple BUG= ========== to ========== Replace base::Tuple with std::tuple BUG=554987 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Replace base::Tuple with std::tuple BUG=554987 ========== to ========== Replace base::Tuple implementation with std::tuple * Remove base::Tuple and make base::Tuple as an alias of std::tuple. * Expand the alias where it's used in a class template specialization to avoid MSVC2013 internal compiler error. BUG=554987 ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673563002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673563002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tzik@chromium.org changed reviewers: + danakj@chromium.org, thakis@chromium.org
PTAL
Cool, lgtm :-) Maybe the text on chromium-cpp.appspot.com could be a bit longer: https://codereview.chromium.org/1673563002/diff/90001/ipc/ipc_message_utils.h File ipc/ipc_message_utils.h (right): https://codereview.chromium.org/1673563002/diff/90001/ipc/ipc_message_utils.h... ipc/ipc_message_utils.h:661: struct ParamTraits<std::tuple<>> { This change is for 2013, right? https://codereview.chromium.org/1673563002/diff/90001/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/1673563002/diff/90001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:481: <td><a href="https://crbug.com/554987">Tracking bug</a> to plan moving from <code>base::Tuple</code> to <code>std::tuple</code>. See also <code>std::tie</code></td> Should this mention the vs2013 compiler bug and how to work around it? Maybe "base::Tuple is now an alias for std::tuple. In class template specializations, use std::tuple instead of base::Tuple to work around a MSVS2013 crash" (and maybe paste something from the crash so that if one googles for the error this page is found. I'm not sure if the crash has something useful one would google for in it though.)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673563002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673563002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tzik@chromium.org changed reviewers: + piman@chromium.org, tsepez@chromium.org
Updated! Adding tsepez and piman as //ipc owner and //ppapi/proxy owner. Could you take a look? https://codereview.chromium.org/1673563002/diff/90001/ipc/ipc_message_utils.h File ipc/ipc_message_utils.h (right): https://codereview.chromium.org/1673563002/diff/90001/ipc/ipc_message_utils.h... ipc/ipc_message_utils.h:661: struct ParamTraits<std::tuple<>> { On 2016/02/09 12:23:11, Nico wrote: > This change is for 2013, right? Yes, it seems failing on the type alias + variadic template + specialization of class template. https://codereview.chromium.org/1673563002/diff/90001/styleguide/c++/c++11.html File styleguide/c++/c++11.html (right): https://codereview.chromium.org/1673563002/diff/90001/styleguide/c++/c++11.ht... styleguide/c++/c++11.html:481: <td><a href="https://crbug.com/554987">Tracking bug</a> to plan moving from <code>base::Tuple</code> to <code>std::tuple</code>. See also <code>std::tie</code></td> On 2016/02/09 12:23:11, Nico wrote: > Should this mention the vs2013 compiler bug and how to work around it? > > Maybe "base::Tuple is now an alias for std::tuple. In class template > specializations, use std::tuple instead of base::Tuple to work around a MSVS2013 > crash" (and maybe paste something from the crash so that if one googles for the > error this page is found. I'm not sure if the crash has something useful one > would google for in it though.) Done
lgtm
lgtm
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673563002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673563002/130001
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 tzik@chromium.org
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/1673563002/#ps130001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673563002/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673563002/130001
Message was sent while issue was closed.
Description was changed from ========== Replace base::Tuple implementation with std::tuple * Remove base::Tuple and make base::Tuple as an alias of std::tuple. * Expand the alias where it's used in a class template specialization to avoid MSVC2013 internal compiler error. BUG=554987 ========== to ========== Replace base::Tuple implementation with std::tuple * Remove base::Tuple and make base::Tuple as an alias of std::tuple. * Expand the alias where it's used in a class template specialization to avoid MSVC2013 internal compiler error. BUG=554987 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
On 2016/02/11 10:26:38, commit-bot: I haz the power wrote: > Committed patchset #8 (id:130001) We will need to revert part or all of this, or modify it, in order to fix a build break with VS 2015. the FYI builders are unhappy: https://build.chromium.org/p/chromium.fyi/builders/Chromium%20Builder
Message was sent while issue was closed.
That's an ICE and we need to wait for update 2 anyhow. Can you report this with ms? It seems backwards that we can use less modern stuff with a _newer_ msvs.
Message was sent while issue was closed.
(if this does get reverted, https://codereview.chromium.org/1689093002/ needs to be reverted too)
Message was sent while issue was closed.
sebmarchand@chromium.org changed reviewers: + sebmarchand@chromium.org
Message was sent while issue was closed.
I'm trying to find a fix for this (without having to do a full revert), but if there's no clean fix we'll probably have to revert it: - Having this compile issue will maybe hide some other ones, we should try to keep the VS2015 builder green even if we need to wait for update 2 before shipping. - The PGO builder (on the official waterfall) is using VS2015 and will also break.
Message was sent while issue was closed.
Please, as the first thing you do, make a reduced repro, and send it to the MSVS team, so that there's a chance this gets fixed.
Message was sent while issue was closed.
On 2016/02/11 18:36:15, Sébastien Marchand wrote: > I'm trying to find a fix for this (without having to do a full revert), but if > there's no clean fix we'll probably have to revert it: > - Having this compile issue will maybe hide some other ones, we should try to > keep the VS2015 builder green even if we need to wait for update 2 before > shipping. > - The PGO builder (on the official waterfall) is using VS2015 and will also > break. Oh... I'm surprised to know there's new ICE on the new compiler... But, this is a known pattern of ICE. Can I try a quick fix to this?
Message was sent while issue was closed.
Sure, I'm trying to find a minimal repro for this. Let's move the discussion to crbug.com/586187
Message was sent while issue was closed.
Description was changed from ========== Replace base::Tuple implementation with std::tuple * Remove base::Tuple and make base::Tuple as an alias of std::tuple. * Expand the alias where it's used in a class template specialization to avoid MSVC2013 internal compiler error. BUG=554987 ========== to ========== Replace base::Tuple implementation with std::tuple * Remove base::Tuple and make base::Tuple as an alias of std::tuple. * Expand the alias where it's used in a class template specialization to avoid MSVC2013 internal compiler error. BUG=554987 Committed: https://crrev.com/9ca3021996e87ad03adf4451fd7fc7f8097c8f46 Cr-Commit-Position: refs/heads/master@{#374878} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9ca3021996e87ad03adf4451fd7fc7f8097c8f46 Cr-Commit-Position: refs/heads/master@{#374878} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
