base::Optional: Use anonymous union instead of base::AlignedMemory

base::Optional: Use anonymous union instead of base::AlignedMemory

Because it's simpler, and accomplishes the same thing. Also modify the
C++11 allowed features list to allow unions with class members, to
make this change style guide compliant. :-)

This is adapted from https://codereview.webrtc.org/2071003003/ by
ossu@webrtc.org (which adapted the technique from WebRTC's
rtc::Optional, where it has been in use since May 9:
https://crrev.com/d040480f69cc6fe65dd101c493d0561a0cdbaa4a).

Committed: https://crrev.com/882859a7237ac08da724cabfc863e44f9e63f520
Cr-Commit-Position: refs/heads/master@{#412199}

[kwiberg-chromium, 2016-06-18 05:27:13]

kwiberg@chromium.org changed reviewers:
+ danakj@chromium.org, mlamouri@chromium.org

[danakj, 2016-06-22 23:00:04]

https://codereview.chromium.org/2080003002/diff/1/base/optional.h
File base/optional.h (right):

https://codereview.chromium.org/2080003002/diff/1/base/optional.h#newcode47
base/optional.h:47: T value_;
Why does libcxx put a second field in the union?
https://github.com/llvm-mirror/libcxx/blob/master/include/experimental/option...

And then initialize it instead in the default constructor:
https://github.com/llvm-mirror/libcxx/blob/master/include/experimental/option...

[danakj, 2016-06-22 23:01:08]

Also, can you send an email to cxx@chromium.org about using unions with class
members? That way others can follow along who are interested. You can point to
this CL as a good example usage.

[danakj, 2016-06-22 23:01:37]

On 2016/06/22 23:01:08, danakj wrote:
> Also, can you send an email to mailto:cxx@chromium.org about using unions with
class
> members? That way others can follow along who are interested. You can point to
> this CL as a good example usage.

See the 2nd paragraph on http://chromium-cpp.appspot.com/ about that.

[kwiberg-chromium, 2016-06-23 02:18:49]

https://codereview.chromium.org/2080003002/diff/1/base/optional.h
File base/optional.h (right):

https://codereview.chromium.org/2080003002/diff/1/base/optional.h#newcode47
base/optional.h:47: T value_;
On 2016/06/22 23:00:03, danakj wrote:
> Why does libcxx put a second field in the union?
>
https://github.com/llvm-mirror/libcxx/blob/master/include/experimental/option...
> 
> And then initialize it instead in the default constructor:
>
https://github.com/llvm-mirror/libcxx/blob/master/include/experimental/option...

It seems like it's required to make constexpr construction work fully:
https://akrzemi1.wordpress.com/2012/12/13/constexpr-unions/. (The code described
in that article isn't identical to what's in libcxx, though, but it seems to me
like they'd work almost the same.)

But the implementation here doesn't try to provide constexpr constructors. Or so
it claims---but I see an awful lot of "constexpr"s in this file...

I think adding an extra dummy member can probably wait until there are unit
tests that try to prove that constexpr construction works; until then, it'd just
be useless noise that maybe doesn't even work because it isn't being tested.

[ossu-chromium, 2016-06-23 08:20:06]

> It seems like it's required to make constexpr construction work fully:
> https://akrzemi1.wordpress.com/2012/12/13/constexpr-unions/. (The code
described
> in that article isn't identical to what's in libcxx, though, but it seems to
me
> like they'd work almost the same.)

Hmm... I like the solution with dummy_t (though obviously, it should be called
empty_t). If this change to using a union doesn't reduce the functionality of
base::Optional, I also think it's not required for this CL, but rather should be
added separately.

[ossu-chromium, 2016-06-23 13:44:06]

On 2016/06/23 08:20:06, ossu1 wrote:
> Hmm... I like the solution with dummy_t (though obviously, it should be called
> empty_t). If this change to using a union doesn't reduce the functionality of
> base::Optional, I also think it's not required for this CL, but rather should
be
> added separately.

Actually, it seems the variant using a char that gets zero-initialized stops at
least GCC from complaining about some usages of Optional that an empty struct
(or nothing) doesn't do. The WebRTC version has been updated with that and I
expect kwiberg will add a patch set to this one doing that as well.

[danakj, 2016-06-23 18:52:11]

On 2016/06/23 13:44:06, ossu1 wrote:
> On 2016/06/23 08:20:06, ossu1 wrote:
> > Hmm... I like the solution with dummy_t (though obviously, it should be
called
> > empty_t). If this change to using a union doesn't reduce the functionality
of
> > base::Optional, I also think it's not required for this CL, but rather
should
> be
> > added separately.
> 
> Actually, it seems the variant using a char that gets zero-initialized stops
at
> least GCC from complaining about some usages of Optional that an empty struct
> (or nothing) doesn't do. The WebRTC version has been updated with that and I
> expect kwiberg will add a patch set to this one doing that as well.

Thanks! I'll wait for that.

[PEConn, 2016-06-27 08:07:19]

peconn@chromium.org changed reviewers:
+ peconn@chromium.org

[PEConn, 2016-06-27 08:07:20]

You'll need to update docs/optional.md as it contains references to it being
implemented with base::AlignedMemory.

[mlamouri (slow - plz ping), 2016-06-27 12:29:37]

Removing myself from the reviewers list. I'm very happy with danakj@ taking care
of this.

[mlamouri (slow - plz ping), 2016-06-28 13:14:09]

mlamouri@chromium.org changed reviewers:
- mlamouri@chromium.org

[kwiberg-chromium, 2016-08-15 10:02:51]

The CQ bit was checked by kwiberg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 10:03:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 11:15:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-15 11:15:09]

Dry run: This issue passed the CQ dry run.

[kwiberg-chromium, 2016-08-15 11:17:49]

I think I've tied up all the loose ends now. PTAL.

[danakj, 2016-08-15 21:22:58]

Couple nits LGTM tho

https://codereview.chromium.org/2080003002/diff/60001/base/optional.h
File base/optional.h (right):

https://codereview.chromium.org/2080003002/diff/60001/base/optional.h#newcode47
base/optional.h:47: // empty_ exists so that the union will always be
initialized, even when it
nit: || around variable names. "|empty_| exists.."

https://codereview.chromium.org/2080003002/diff/60001/base/optional.h#newcode57
base/optional.h:57: OptionalStorage() {};
no ; here

also or how about =default; instead?

https://codereview.chromium.org/2080003002/diff/60001/base/optional.h#newcode65
base/optional.h:65: // empty_ exists so that the union will always be
initialized, even when it
nitto

[kwiberg-chromium, 2016-08-16 08:23:18]

The CQ bit was checked by kwiberg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 08:23:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kwiberg-chromium, 2016-08-16 08:23:47]

https://codereview.chromium.org/2080003002/diff/60001/base/optional.h
File base/optional.h (right):

https://codereview.chromium.org/2080003002/diff/60001/base/optional.h#newcode47
base/optional.h:47: // empty_ exists so that the union will always be
initialized, even when it
On 2016/08/15 21:22:57, danakj wrote:
> nit: || around variable names. "|empty_| exists.."

Done.

https://codereview.chromium.org/2080003002/diff/60001/base/optional.h#newcode57
base/optional.h:57: OptionalStorage() {};
On 2016/08/15 21:22:57, danakj wrote:
> no ; here

Done.

> also or how about =default; instead?

Doesn't work (and omitting it entirely also doesn't work, for the same reason).
http://en.cppreference.com/w/cpp/language/union says: "If a union contains a
non-static data member with a non-trivial special member function (default
constructor, copy/move constructor, copy/move assignment, or destructor), that
function is deleted by default in the union and needs to be defined explicitly
by the programmer."

I guess if we were willing to double the number of definitions for
OptionalStorage once more, we might be able to not declare constructors for Ts
with trivial default constructors, but there would be no point in doing that
since the OptionalStorage default constructor is going to be nontrivial anyway
because we need to initialize is_null_.

https://codereview.chromium.org/2080003002/diff/60001/base/optional.h#newcode65
base/optional.h:65: // empty_ exists so that the union will always be
initialized, even when it
On 2016/08/15 21:22:57, danakj wrote:
> nitto

Ha! Done.

[commit-bot: I haz the power, 2016-08-16 09:35:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 09:35:35]

Dry run: This issue passed the CQ dry run.

[kwiberg-chromium, 2016-08-16 09:38:39]

The CQ bit was checked by kwiberg@chromium.org

[kwiberg-chromium, 2016-08-16 09:38:39]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2080003002/#ps80001
(title: "review nits")

[commit-bot: I haz the power, 2016-08-16 09:38:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 09:42:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-16 09:44:22]

Description was changed from

==========
base::Optional: Use anonymous union instead of base::AlignedMemory

Because it's simpler, and accomplishes the same thing. Also modify the
C++11 allowed features list to allow unions with class members, to
make this change style guide compliant. :-)

This is adapted from https://codereview.webrtc.org/2071003003/ by
ossu@webrtc.org (which adapted the technique from WebRTC's
rtc::Optional, where it has been in use since May 9:
https://crrev.com/d040480f69cc6fe65dd101c493d0561a0cdbaa4a).
==========

to

==========
base::Optional: Use anonymous union instead of base::AlignedMemory

Because it's simpler, and accomplishes the same thing. Also modify the
C++11 allowed features list to allow unions with class members, to
make this change style guide compliant. :-)

This is adapted from https://codereview.webrtc.org/2071003003/ by
ossu@webrtc.org (which adapted the technique from WebRTC's
rtc::Optional, where it has been in use since May 9:
https://crrev.com/d040480f69cc6fe65dd101c493d0561a0cdbaa4a).

Committed: https://crrev.com/882859a7237ac08da724cabfc863e44f9e63f520
Cr-Commit-Position: refs/heads/master@{#412199}
==========

[commit-bot: I haz the power, 2016-08-16 09:44:23]

Patchset 5 (id:??) landed as
https://crrev.com/882859a7237ac08da724cabfc863e44f9e63f520
Cr-Commit-Position: refs/heads/master@{#412199}