Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2974603002:
[Protobuf] Add <map> include to workaround a gcc bug (Closed)
|
Created:
3 years, 5 months ago by Tom Anderson Modified:
3 years, 5 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Protobuf] Add <map> include to workaround a gcc bug
GCC gets a bit confused by libc++'s map headers, and including <map>
after <set> can cause problems under certain conditions [1].
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81347
BUG=webrtc:7922
R=pkasting@chromium.org,dpranke@chromium.org
Review-Url: https://codereview.chromium.org/2974603002
Cr-Commit-Position: refs/heads/master@{#486608}
Committed: https://chromium.googlesource.com/chromium/src/+/5573361f7b2cf91b5b4f38d3c277e8687b7a60de
Patch Set 1 #
Total comments: 2
Patch Set 2 : forward diff #Patch Set 3 : Add no-op change to mb.py #
Messages
Total messages: 52 (40 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Protobuf] Add <map> include to workaround a gcc bug BUG=webrtc:7922 R=pkasting@chromium.org ========== to ========== [Protobuf] Add <map> include to workaround a gcc bug GCC gets a bit confused by libc++'s map headers, and including <map> after <set> can cause problems under certain conditions [1]. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81347 BUG=webrtc:7922 R=pkasting@chromium.org ==========
The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run
pkasting@ ptal
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
It looks to me like the source file didn't need this #include anyway. Have you submitting a PR upstream to libprotobuf? Any chance we can just get that merged and pull a new version? https://codereview.chromium.org/2974603002/diff/20001/third_party/protobuf/pa... File third_party/protobuf/patches/0012-add-map-include.patch (right): https://codereview.chromium.org/2974603002/diff/20001/third_party/protobuf/pa... third_party/protobuf/patches/0012-add-map-include.patch:8: -#include <map> Is this a reversed delta?
On 2017/07/06 23:19:23, Peter Kasting wrote: > It looks to me like the source file didn't need this #include anyway. It didn't, but the thing that triggers the gcc bug is this pattern: 1. #include <set> 2. //use set 3. #include <map> 4. 'using namespace std' or 'using std::map'. descriptor.h is 1 and 2 which gets included from any other file that adds 3 and 4. For some reason, adding the <map> include first fixes it ¯\_(ツ)_/¯ > Have you > submitting a PR upstream to libprotobuf? Any chance we can just get that merged > and pull a new version? I believe this is already being fixed upstream since the 'using namespace std' is removed. I believe updating protobuf would be a challenge given how behind we are from upstream. https://codereview.chromium.org/2974603002/diff/20001/third_party/protobuf/pa... File third_party/protobuf/patches/0012-add-map-include.patch (right): https://codereview.chromium.org/2974603002/diff/20001/third_party/protobuf/pa... third_party/protobuf/patches/0012-add-map-include.patch:8: -#include <map> On 2017/07/06 23:19:23, Peter Kasting wrote: > Is this a reversed delta? Yep, oops
On 2017/07/06 23:51:08, Tom Anderson wrote: > I believe updating protobuf would be a challenge given how behind > we are from upstream. D: Do you have to get this in urgently? It would be nice to try and pull a new upstream version. In the past my answer to all "patch it downstream because it's easier" requests was "no, please go ahead and pull a new version", and every time I relax the bar on that it makes the bar even more difficult to clear in future patches. I'm not going to absolutely refuse this -- LGTM to land -- but please don't Tragedy Of The Commons our libprotobuf copy :(
On 2017/07/06 23:54:04, Peter Kasting wrote: > On 2017/07/06 23:51:08, Tom Anderson wrote: > > I believe updating protobuf would be a challenge given how behind > > we are from upstream. > > D: > > Do you have to get this in urgently? It would be nice to try and pull a new > upstream version. In the past my answer to all "patch it downstream because > it's easier" requests was "no, please go ahead and pull a new version", and > every time I relax the bar on that it makes the bar even more difficult to clear > in future patches. > > I'm not going to absolutely refuse this -- LGTM to land -- but please don't > Tragedy Of The Commons our libprotobuf copy :( Not sure how long it takes to merge the changes to the GitHub repo, but the bugs from the other CL have been fixed for over a day, so I'm just going to land this CL and include the revert in the one that rolls protobuf (I've already let the protobuf team know that we need the changes propagated to the public GitHub repo)
The CQ bit was checked by thomasanderson@chromium.org
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
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@google.com 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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@google.com 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Weird. linux_chromium_{compile_dbg,rel}_ng keep failing over and over again, in
both with and without patch compile steps. But the build is fine on all my
other CLs
The CQ bit was checked by thomasanderson@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 ========== [Protobuf] Add <map> include to workaround a gcc bug GCC gets a bit confused by libc++'s map headers, and including <map> after <set> can cause problems under certain conditions [1]. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81347 BUG=webrtc:7922 R=pkasting@chromium.org ========== to ========== [Protobuf] Add <map> include to workaround a gcc bug GCC gets a bit confused by libc++'s map headers, and including <map> after <set> can cause problems under certain conditions [1]. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81347 BUG=webrtc:7922 R=pkasting@chromium.org,dpranke@chromium.org ==========
+dpranke for tools/mb/mb.py
The CQ bit was checked by thomasanderson@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...
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@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: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2974603002/#ps60001 (title: "Add no-op change to mb.py")
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": 60001, "attempt_start_ts": 1499995647959860,
"parent_rev": "8e8b54f1bd9d90c56dc7902c8447594fd6624ab1", "commit_rev":
"5573361f7b2cf91b5b4f38d3c277e8687b7a60de"}
Message was sent while issue was closed.
Description was changed from ========== [Protobuf] Add <map> include to workaround a gcc bug GCC gets a bit confused by libc++'s map headers, and including <map> after <set> can cause problems under certain conditions [1]. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81347 BUG=webrtc:7922 R=pkasting@chromium.org,dpranke@chromium.org ========== to ========== [Protobuf] Add <map> include to workaround a gcc bug GCC gets a bit confused by libc++'s map headers, and including <map> after <set> can cause problems under certain conditions [1]. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81347 BUG=webrtc:7922 R=pkasting@chromium.org,dpranke@chromium.org Review-Url: https://codereview.chromium.org/2974603002 Cr-Commit-Position: refs/heads/master@{#486608} Committed: https://chromium.googlesource.com/chromium/src/+/5573361f7b2cf91b5b4f38d3c277... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/5573361f7b2cf91b5b4f38d3c277... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
