Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2090223003:
Added an empty member to the union of rtc::Optional, so that it is always initializable. (Closed)
DescriptionAdded an empty member to the union of rtc::Optional, so that it is always initializable.
Added notry due to flaky android_dbg bot.
NOTRY=true
BUG=webrtc:6033
Committed: https://crrev.com/ea41694e08a31c4001e600773e47b4fc693cc539
Cr-Commit-Position: refs/heads/master@{#13343}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Removed else clauses; added explanatory comment. #Messages
Total messages: 25 (12 generated)
https://codereview.webrtc.org/2090223003/diff/1/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2090223003/diff/1/webrtc/base/optional.h#newcode76 webrtc/base/optional.h:76: else I'm not 100% sure these else-statements are necessary. They weren't needed to silence the compiler warning in the issue description, but that one seemed kind of spurious, so maybe a different set of circumstances could still get it to warn without these initializers.
Description was changed from ========== Added an empty member to the union of rtc::Optional, so that it is always initializable. BUG=webrtc:6033 ========== to ========== Added an empty member to the union of rtc::Optional, so that it is always initializable. BUG=webrtc:6033 ==========
ossu@webrtc.org changed reviewers: + kwiberg@webrtc.org
ossu@webrtc.org changed reviewers: + tommi@webrtc.org
lgtm - would be good if we don't need these else statements. Also, if you could add a comment that explains why we have that extra union member, it can help us avoid a regression (better yet, write a regression test)
https://codereview.webrtc.org/2090223003/diff/1/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2090223003/diff/1/webrtc/base/optional.h#newcode76 webrtc/base/optional.h:76: else On 2016/06/23 14:08:03, ossu wrote: > I'm not 100% sure these else-statements are necessary. They weren't needed to > silence the compiler warning in the issue description, but that one seemed kind > of spurious, so maybe a different set of circumstances could still get it to > warn without these initializers. Possibly. But it seems more reasonable to add workarounds only after we think we need them. https://codereview.webrtc.org/2090223003/diff/1/webrtc/base/optional.h#newcod... webrtc/base/optional.h:202: char empty_; 1. libcxx put the dummy member first. I think that's because the first member is the one that gets zero-initialized. 2. I liked "dummy" better than "empty", because we don't consistently write to it when we switch from being filled to not filled. It's really just a hack to make the language do what we want. 3. As Tommi said, big fat comment here please.
https://codereview.webrtc.org/2090223003/diff/1/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2090223003/diff/1/webrtc/base/optional.h#newcode76 webrtc/base/optional.h:76: else On 2016/06/24 05:37:39, kwiberg-webrtc wrote: > On 2016/06/23 14:08:03, ossu wrote: > > I'm not 100% sure these else-statements are necessary. They weren't needed to > > silence the compiler warning in the issue description, but that one seemed > kind > > of spurious, so maybe a different set of circumstances could still get it to > > warn without these initializers. > > Possibly. But it seems more reasonable to add workarounds only after we think we > need them. Well, with the copy/move initialization, we'd sometimes have non-initialized unions without them. However, libcxx doesn't seem to handle that case either. In all honesty, I feel GCC is probably in the wrong here - warning for something that cannot happen. I'll remove the else clauses for now. https://codereview.webrtc.org/2090223003/diff/1/webrtc/base/optional.h#newcod... webrtc/base/optional.h:202: char empty_; On 2016/06/24 05:37:39, kwiberg-webrtc wrote: > 1. libcxx put the dummy member first. I think that's because the first member is > the one that gets zero-initialized. I don't believe the order matters, since it's a union. We're also zero-initializing a specific member of the union. > 2. I liked "dummy" better than "empty", because we don't consistently write to > it when we switch from being filled to not filled. It's really just a hack to > make the language do what we want. libcxx has __null_state_, Andrzej's blog has dummy, but uses an empty struct. What's in a name? :) > 3. As Tommi said, big fat comment here please. Alright.
On 2016/06/23 19:57:19, tommi-webrtc wrote: > lgtm - would be good if we don't need these else statements. Also, if you could > add a comment that explains why we have that extra union member, it can help us > avoid a regression (better yet, write a regression test) Hmm. I'm not sure how I'd go about creating a regression test for this, since it seems only to be triggered on one compiler, and only for some usages. In the case of webrtc:6033, for example, the code reads an optional twice using operator*, both within an if-clause to make sure the optional has a value; only the second of those calls is flagged by the compiler. The only reasoning I can come up with why, is that the compiler believes the first call might alter the optional, causing the compiler to ignore any knowledge it has about it and concluding that it might not have a value at that point. I'll give it a think, though.
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2090223003/#ps20001 (title: "Removed else clauses; added explanatory comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14614)
The CQ bit was checked by ossu@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14665)
Description was changed from ========== Added an empty member to the union of rtc::Optional, so that it is always initializable. BUG=webrtc:6033 ========== to ========== Added an empty member to the union of rtc::Optional, so that it is always initializable. Added notry due to flaky android_dbg bot. NOTRY=true BUG=webrtc:6033 ==========
The CQ bit was checked by ossu@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Added an empty member to the union of rtc::Optional, so that it is always initializable. Added notry due to flaky android_dbg bot. NOTRY=true BUG=webrtc:6033 ========== to ========== Added an empty member to the union of rtc::Optional, so that it is always initializable. Added notry due to flaky android_dbg bot. NOTRY=true BUG=webrtc:6033 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Added an empty member to the union of rtc::Optional, so that it is always initializable. Added notry due to flaky android_dbg bot. NOTRY=true BUG=webrtc:6033 ========== to ========== Added an empty member to the union of rtc::Optional, so that it is always initializable. Added notry due to flaky android_dbg bot. NOTRY=true BUG=webrtc:6033 Committed: https://crrev.com/ea41694e08a31c4001e600773e47b4fc693cc539 Cr-Commit-Position: refs/heads/master@{#13343} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ea41694e08a31c4001e600773e47b4fc693cc539 Cr-Commit-Position: refs/heads/master@{#13343} |
