base::Optional not compatible with memset(0) |
||
Issue descriptionWhile investigating a bug, I found that a base::Optional variable was being initialized with memset(0). This was indirect -- they were initializing one class that way and I happened to be adding a base::Optional member to the class. base::Optional pipes a bit until it gets down to storage_.is_null_. That is where memset(0) was a problem. is_null_ = 0; is the equivalent of saying this base::Optional *DOES* have a value. To resolve this, I'm making the boolean an affirmative expression rather than a negative expression. is_null_ is a negating expression, similar to "is not". Instead, I will replace it with is_populated_, which is an affirmative expression. When someone memset(0)s this affirmative expression, is_populated_ = 0; will mean what is expected (this Optional does not hold a value).
,
Jan 24 2018
I have a patch out here: https://chromium-review.googlesource.com/c/chromium/src/+/883946 Re #1: I agree...but I also think this would be worth it. In general, memset(0)ing an object feels like a very C-with-classes way to code C++. The default initializer should (and does) do the right thing here. So in that way, I agree with you. However, it is very easy for this mistake to slip in for people coming from C. And it seems rather easy to fix as well. I guess I would almost call it a feature rather than a bug: "Preserves memset(0) semantics".
,
Jan 24 2018
For context, here is how I found it: https://chromium-review.googlesource.com/c/chromium/src/+/861643/17/content/browser/web_contents/aura/overscroll_navigation_overlay_unittest.cc#b254 The class was being memset(0)ed. I added a base::Optional member to that class.
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0a5dd14ae173846fb5d67408790b7df4e44f38b6 commit 0a5dd14ae173846fb5d67408790b7df4e44f38b6 Author: Chris Blume <cblume@chromium.org> Date: Wed Jan 24 22:35:40 2018 Use affirmative expression in base::Optional base::Optional has a negative expression to represent if a value is being stored: storage_.is_null_. Because of this, memset(0) on a base::Optional will mark that optional as actually storing a value (when it clearly doesn't). Using memset(0) on base::Optional sounds a bit dirty but it can easily happen indirectly. Someone might memset(0) a class with a private member of base::Optional. Change the expression to be affirmative to preserve memset(0) intention using storage_.is_populated_. BUG= 805565 Change-Id: I9c5b85cdaa58960f15809160f2d0de6d0cc52c7b Reviewed-on: https://chromium-review.googlesource.com/883946 Reviewed-by: danakj <danakj@chromium.org> Commit-Queue: Chris Blume <cblume@chromium.org> Cr-Commit-Position: refs/heads/master@{#531722} [modify] https://crrev.com/0a5dd14ae173846fb5d67408790b7df4e44f38b6/base/optional.h
,
Jan 24 2018
|
||
►
Sign in to add a comment |
||
Comment 1 by fsamuel@google.com
, Jan 24 2018