Remove disabling -Warray-bounds for GCC in FBString.h #999
Conversation
There was a problem hiding this comment.
@yfeldblum has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@yfeldblum does this PR also have to wait until the remaining internal uses of GCC 4.9 move on to GCC 5+? |
|
Ping @lbrandy in case this is blocked in the internal review as well. Would appreciate the review. Last I heard from @yfeldblum, there were a few internal things relying on GCC 4.9, so we may not quite be able to land this PR until they move over to GCC 5+. Let me know if that is the case so we can proceed accordingly. |
|
Hey @JoeLoser, there is some internal build configuration which I think is gcc5 but which doesn't agree and is showing build-time failures that I don't quite understand it yet. |
|
Also fwiw, I don't see anything internally atm which still uses gcc49 depending on this. |
|
Thanks for the update and looking into this a bit @yfeldblum. After rereading some of the comments on the GCC bug report (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59124), my takeaway was that that bug is "mostly fixed" in what made the GCC 5 branch, but there are some lingering issues with it that remain even in GCC 8 (but were fine in GCC 6/7). If we're hitting one of those cases that have not yet been fixed, it seems like we should keep the disabling of the warning in, but not bound it to GCC versions 5 or less (https://github.com/facebook/folly/pull/999/files#diff-614a0f21886a05769b4e7c183635055aL105) |
|
Yeah that's fine. At least keep the suppression in FixedString.h but we can try removing the suppression only in FBString.h. Want to update the PR? |
Summary: - Existing code disables `-Warray-bounds` warning for GCC due to a bug in GCC 4.9. - As seen in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59124, the bug has been resolved for GCC 5.1. - Since the bug has been resolved, we no longer need to disable the warning.
JoeLoser
force-pushed
the
fbstring_array_bounds_gcc49
branch
from
f68543b
to
3d40394
Compare
JoeLoser
changed the title
|
Sure, I kept |
There was a problem hiding this comment.
@yfeldblum has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: - Existing code disables `-Warray-bounds` warning for GCC due to a bug in GCC 4.9. - As seen in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59124, the bug has been resolved for GCC 5.1. - Since the bug has been resolved, we no longer need to disable the warning. yfeldblum I suspect that we have to hold off on this PR until the internal use cases relying on GCC 4.9 upgrade, but I figured it is better to have some of these cleanup opportunities ready for when the time arises. Pull Request resolved: facebook#999 Reviewed By: ot, Orvid Differential Revision: D13729723 Pulled By: yfeldblum fbshipit-source-id: da33704c2e2022c2dad0681e4d6152105a1cf54d
Summary:
-Warray-boundswarning for GCC due to a bug inGCC 4.9.
has been resolved for GCC 5.1.
warning.
@yfeldblum I suspect that we have to hold off on this PR until the internal use cases relying on GCC 4.9 upgrade, but I figured it is better to have some of these cleanup opportunities ready for when the time arises.