Make base::Optional trivially destructible when possible.

Make base::Optional trivially destructible when possible.

* Implement base::is_trivially_destructible.
* Add value_type to Optional.
* Make Optional<T> trivially destructible when T is trivially destructible.
* Add some more tests on Optional.

BUG=521269, 554293
Committed: https://crrev.com/f06b07dffd6ac5ee2f4e8bd2e1385c04a34f3da4
Cr-Commit-Position: refs/heads/master@{#396423}

[alshabalin, 2016-05-22 08:07:17]

Description was changed from

==========
Make base::Optional trivially destructible when possible.

* Implement base::is_trivially_destructible.
* Add value_type to Optional.
* Make Optional<T> trivially destructible when T is trivially destructible.
* Add some more tests on Optional.

BUG=521269
==========

to

==========
Make base::Optional trivially destructible when possible.

* Implement base::is_trivially_destructible.
* Add value_type to Optional.
* Make Optional<T> trivially destructible when T is trivially destructible.
* Add some more tests on Optional.

BUG=521269
==========

[alshabalin, 2016-05-22 08:07:17]

alshabalin@yandex-team.ru changed reviewers:
+ danakj@chromium.org

[alshabalin, 2016-05-24 10:46:33]

PTAL

[alshabalin, 2016-05-25 10:24:40]

alshabalin@yandex-team.ru changed reviewers:
+ thakis@chromium.org

[alshabalin, 2016-05-25 10:28:34]

Dana, Nico PTAL.

[alshabalin, 2016-05-25 10:35:46]

alshabalin@yandex-team.ru changed reviewers:
+ mark@chromium.org, thestig@chromium.org

[alshabalin, 2016-05-25 10:37:37]

+ Lei, Mark.

[Nico, 2016-05-25 13:56:59]

thakis@chromium.org changed reviewers:
+ mlamouri@chromium.org
- mark@chromium.org, thakis@chromium.org, thestig@chromium.org

[Nico, 2016-05-25 13:56:59]

one base reviewer is enough. removing everyone but danakj, adding mlamouri,
author of this file

https://codereview.chromium.org/2000043002/diff/1/base/template_util.h
File base/template_util.h (right):

https://codereview.chromium.org/2000043002/diff/1/base/template_util.h#newcode19
base/template_util.h:19: namespace std {
defining stuff in namespace std in user code has undefined behavior, we
shouldn't do that

[danakj, 2016-05-25 20:02:09]

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

https://codereview.chromium.org/2000043002/diff/1/base/optional.h#newcode49
base/optional.h:49: ~OptionalStorage() = default;
Can you leave a comment here explaining that this is not calling the destructor
on T and why.

https://codereview.chromium.org/2000043002/diff/1/base/optional_unittest.cc
File base/optional_unittest.cc (right):

https://codereview.chromium.org/2000043002/diff/1/base/optional_unittest.cc#n...
base/optional_unittest.cc:372: EXPECT_EQ(TestObject::State::MOVE_CONSTRUCTED,
a->state());
I'm a little concerned that this is now testing compiler optimizations, because
it could construct a and then assign to it, but it's optimizing to construct a
from the move(b) directly I guess?

Does this patch pass on debug, release, and official builds? If you can't try
official can you try -Os -O2 and -O0?

I think I'd be more comfortable with just using a MOVED_TO state for both this
and below.

https://codereview.chromium.org/2000043002/diff/1/base/template_util.h
File base/template_util.h (right):

https://codereview.chromium.org/2000043002/diff/1/base/template_util.h#newcod...
base/template_util.h:107: namespace internal {
Sorry I've been thinking about how to do this in a way that isn't defining stuff
in std, which is bad as nico says.

Can you do something like waht this does instead (maybe pull the #if checks out
into a single define so we can use that for both)?

https://codereview.chromium.org/2001783002/diff/100001/base/template_util.h

[alshabalin, 2016-05-26 09:18:46]

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

https://codereview.chromium.org/2000043002/diff/1/base/optional.h#newcode49
base/optional.h:49: ~OptionalStorage() = default;
On 2016/05/25 20:02:08, danakj wrote:
> Can you leave a comment here explaining that this is not calling the
destructor
> on T and why.

Done.

https://codereview.chromium.org/2000043002/diff/1/base/optional_unittest.cc
File base/optional_unittest.cc (right):

https://codereview.chromium.org/2000043002/diff/1/base/optional_unittest.cc#n...
base/optional_unittest.cc:372: EXPECT_EQ(TestObject::State::MOVE_CONSTRUCTED,
a->state());
On 2016/05/25 20:02:08, danakj wrote:
> I'm a little concerned that this is now testing compiler optimizations,
because
> it could construct a and then assign to it, but it's optimizing to construct a
> from the move(b) directly I guess?
> 
> Does this patch pass on debug, release, and official builds? If you can't try
> official can you try -Os -O2 and -O0?
> 
> I think I'd be more comfortable with just using a MOVED_TO state for both this
> and below.

Why is it an optimization? We default construct |a| which means TestObject is
not constructed at all. Then we call move assignment operator on |a| which calls
InitOrAssign then Init then placement new with move constructor on TestObject.
So MOVE_CONSTRUCTED must be achieved on any path, optimized or not.

Checked Debug, Release, tried with -Os, -O2 and -O0, all on Mac. Tests pass.

If you still want to, I can combine states COPY_CONSTRUCTED and COPY_ASSIGNED
into COPIED_TO, MOVE_CONSTRUCTED and MOVE_ASSIGNED into MOVED_TO. But I think
these extra states are useful in showing that we do really move/copy construct
an underlying object instead of default constructing and then copy/move
assigning to it.

https://codereview.chromium.org/2000043002/diff/1/base/template_util.h
File base/template_util.h (right):

https://codereview.chromium.org/2000043002/diff/1/base/template_util.h#newcode19
base/template_util.h:19: namespace std {
On 2016/05/25 13:56:59, Nico wrote:
> defining stuff in namespace std in user code has undefined behavior, we
> shouldn't do that 

Done. Removed in favor of macro from
https://codereview.chromium.org/2001783002/patch/120001/130004

https://codereview.chromium.org/2000043002/diff/1/base/template_util.h#newcod...
base/template_util.h:107: namespace internal {
On 2016/05/25 20:02:09, danakj wrote:
> Sorry I've been thinking about how to do this in a way that isn't defining
stuff
> in std, which is bad as nico says.
> 
> Can you do something like waht this does instead (maybe pull the #if checks
out
> into a single define so we can use that for both)?
> 
> https://codereview.chromium.org/2001783002/diff/100001/base/template_util.h

Done. Defined a macro CR_USE_FALLBACKS_FOR_OLD_GLIBCXX for that.

[danakj, 2016-05-26 22:43:46]

Description was changed from

==========
Make base::Optional trivially destructible when possible.

* Implement base::is_trivially_destructible.
* Add value_type to Optional.
* Make Optional<T> trivially destructible when T is trivially destructible.
* Add some more tests on Optional.

BUG=521269
==========

to

==========
Make base::Optional trivially destructible when possible.

* Implement base::is_trivially_destructible.
* Add value_type to Optional.
* Make Optional<T> trivially destructible when T is trivially destructible.
* Add some more tests on Optional.

BUG=521269, 554293
==========

[danakj, 2016-05-26 22:57:17]

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

[commit-bot: I haz the power, 2016-05-26 22:57:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000043002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000043002/20001

[danakj, 2016-05-26 22:59:27]

LGTM thanks!

https://codereview.chromium.org/2000043002/diff/1/base/optional_unittest.cc
File base/optional_unittest.cc (right):

https://codereview.chromium.org/2000043002/diff/1/base/optional_unittest.cc#n...
base/optional_unittest.cc:372: EXPECT_EQ(TestObject::State::MOVE_CONSTRUCTED,
a->state());
On 2016/05/26 09:18:45, alshabalin wrote:
> On 2016/05/25 20:02:08, danakj wrote:
> > I'm a little concerned that this is now testing compiler optimizations,
> because
> > it could construct a and then assign to it, but it's optimizing to construct
a
> > from the move(b) directly I guess?
> > 
> > Does this patch pass on debug, release, and official builds? If you can't
try
> > official can you try -Os -O2 and -O0?
> > 
> > I think I'd be more comfortable with just using a MOVED_TO state for both
this
> > and below.
> 
> Why is it an optimization? We default construct |a| which means TestObject is
> not constructed at all. Then we call move assignment operator on |a| which
calls
> InitOrAssign then Init then placement new with move constructor on TestObject.
> So MOVE_CONSTRUCTED must be achieved on any path, optimized or not.
> 
> Checked Debug, Release, tried with -Os, -O2 and -O0, all on Mac. Tests pass.
> 
> If you still want to, I can combine states COPY_CONSTRUCTED and COPY_ASSIGNED
> into COPIED_TO, MOVE_CONSTRUCTED and MOVE_ASSIGNED into MOVED_TO. But I think
> these extra states are useful in showing that we do really move/copy construct
> an underlying object instead of default constructing and then copy/move
> assigning to it.

Ah you're right, I was thinking of the Optional |a| but it's the TestObject
inside. Thanks.

[commit-bot: I haz the power, 2016-05-27 00:41:04]

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

[commit-bot: I haz the power, 2016-05-27 00:41:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[danakj, 2016-05-27 01:19:21]

Needs some changes to build on windows though.

[alshabalin, 2016-05-27 06:14:09]

The CQ bit was checked by alshabalin@yandex-team.ru

[alshabalin, 2016-05-27 06:14:09]

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/2000043002/#ps40001
(title: "Fix build on Windows.")

[commit-bot: I haz the power, 2016-05-27 06:14:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000043002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000043002/40001

[alshabalin, 2016-05-27 06:16:45]

On 2016/05/27 01:19:21, danakj_OOO_back_5.31 wrote:
> Needs some changes to build on windows though.

Yeah, I was wondering why the test above used EXPECT_TRUE(!!a) instead of
EXPECT_TRUE(a). Now I don't.

[commit-bot: I haz the power, 2016-05-27 08:01:44]

Description was changed from

==========
Make base::Optional trivially destructible when possible.

* Implement base::is_trivially_destructible.
* Add value_type to Optional.
* Make Optional<T> trivially destructible when T is trivially destructible.
* Add some more tests on Optional.

BUG=521269, 554293
==========

to

==========
Make base::Optional trivially destructible when possible.

* Implement base::is_trivially_destructible.
* Add value_type to Optional.
* Make Optional<T> trivially destructible when T is trivially destructible.
* Add some more tests on Optional.

BUG=521269, 554293
==========

[commit-bot: I haz the power, 2016-05-27 08:01:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-27 08:03:16]

Description was changed from

==========
Make base::Optional trivially destructible when possible.

* Implement base::is_trivially_destructible.
* Add value_type to Optional.
* Make Optional<T> trivially destructible when T is trivially destructible.
* Add some more tests on Optional.

BUG=521269, 554293
==========

to

==========
Make base::Optional trivially destructible when possible.

* Implement base::is_trivially_destructible.
* Add value_type to Optional.
* Make Optional<T> trivially destructible when T is trivially destructible.
* Add some more tests on Optional.

BUG=521269, 554293

Committed: https://crrev.com/f06b07dffd6ac5ee2f4e8bd2e1385c04a34f3da4
Cr-Commit-Position: refs/heads/master@{#396423}
==========

[commit-bot: I haz the power, 2016-05-27 08:03:17]

Patchset 3 (id:??) landed as
https://crrev.com/f06b07dffd6ac5ee2f4e8bd2e1385c04a34f3da4
Cr-Commit-Position: refs/heads/master@{#396423}