New issue
Advanced search Search tips

Issue 784732 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

base::Optional not updated to the latest version in the standard

Project Member Reported by hidehiko@chromium.org, Nov 14 2017

Issue description

http://en.cppreference.com/w/cpp/utility/optional/operator_cmp

(19)-(30) are missing in base/optional.h.
These will be useful for some specific situations. E.g.;

base::Optional<double> value = ...;
if (value == 0) {
  ...
}

or EXPECT/ASSERT macros in gtest with crbug.com/741584.

 

Comment 1 by danakj@chromium.org, Nov 14 2017

Cc: ice...@yandex-team.ru dcheng@chromium.org mlamouri@chromium.org

Comment 2 by danakj@chromium.org, Nov 14 2017

Status: Available (was: Untriaged)
I believe they are implemented, see: https://cs.chromium.org/chromium/src/base/optional.h?rcl=bf228a619a3edcb3321e1aee0ba23536427e0721&l=405

You will find the tests around https://cs.chromium.org/chromium/src/base/optional_unittest.cc?rcl=bf228a619a3edcb3321e1aee0ba23536427e0721&l=581

Is there something else that is needed or is there an issue with the implementation?
I misunderstood these were missing, but it seems like the signatures aren't matched to the standard?

The Chromium defines

template<typename T> bool operator==(const base::Optional<T>& a, const T& b);

while the standard defines

template<typename T, typename U> bool operator==(const optional<T>& a, U&& b);

So, the standard one can take an arg of the type which is different from the one for Optional's type argument.
Also, the latter captures the second param by universal reference, which looks more generic.

And, I found that same thing can be said to the operator== for two optional value comparator, i.e.

operator==(const Optional<T>& a, const Optional<T>& b) v.s. operator==(const optional<T>& a, const optional<U>& b

Comment 5 by danakj@chromium.org, Nov 14 2017

Ah, the standard made some changes after we implemented optional and we haven't brought optional up to the latest version of the standard. This is one of them.

Comment 6 by danakj@chromium.org, Nov 14 2017

Summary: base::Optional not updated to the latest version in the standard (was: base::Optional has missing comparator operator definitions.)

Comment 7 by danakj@chromium.org, Nov 14 2017

FWIW though, I *think* all the changes made to the standard increase the number of valid things you can do with Optional, so at least we're forward compatible still, AFAIK.
Components: Internals>Core
Great to know that Optional was older than std.

So, I created a WIP CL to move discussion forward.
Can we have something like this?
https://chromium-review.googlesource.com/c/chromium/src/+/771493
# Note: failing tests look unrelated.
There's a bunch of other pieces that have changed in the latest iteration of the class, and I don't know that it is a good strategy to change only the operators without changing the rest or we may create surprising behaviour.

On that CL, the casts uses are not in agreement with the style guide https://google.github.io/styleguide/cppguide.html#Casting, and I didn't see why the bodies are changing at all.
What do you think about possible code bloat due to template<typename T, typename U> signature?
If we have code like

base::Optional<double> value = ...;
if (value == 0) {
}

doesn't that mean that operator==(const Optional<double>&, const int&) will be generated? Please, correct me if I'm wrong.
Yeah, it would. But I'm not sure what you're proposing either. You'd need to bring it to the C++ spec folks if you want to change it. I'm not sure how much code would actually be generated tho since that would just call operator== for double, it might be compiled away.
Re: #10.

So, should we create a CL to make a change at once to catch up with the current C++17 spec? I found diffs on ctor, operator=, emplace, and make_optional. Do you have more things we need to take care?

> changing the body.

The change was to adapt
http://en.cppreference.com/w/cpp/utility/optional/operator_cmp#Return_value

As for bool-style cast, has_value() is alternative, as these should be same.

As for changing the impl, it's for more compatibility with the std.
There seem several edge cases in the current implementation.
E.g., if T defines operator== but not operator!=,
base::Optional<T> a = ..., b = ...;
a != b
can compile, while it cannot with std::optional.

Re: #12.
> that would just call operator== for double, it might be compiled away.

Unfortunately, not with the current base::Optional as follows:

error: invalid operands to binary expression ('int' and 'Optional<double>')

T is inferred to 'double' from the first arg, and 'int' from the second arg,
then the compiler reports their mismatch as an error.

> Do you have more things we need to take care?

Look at the history on cppreference.com since April 2016. For example, constructors: http://en.cppreference.com/mwiki/index.php?title=cpp%2Futility%2Foptional%2Foptional&diff=96683&oldid=85620

> As for changing the impl, it's for more compatibility with the std.
> There seem several edge cases in the current implementation.

Ah thanks, that may have changed, I see some proposals about comparators that have gone by since.

> Unfortunately, not with the current base::Optional as follows:

The question was for comparison with the templated U type which is in the newer proposal.
>> Do you have more things we need to take care?
>
> Look at the history on cppreference.com since April 2016. For example, constructors: http://en.cppreference.com/mwiki/index.php?title=cpp%2Futility%2Foptional%2Foptional&diff=96683&oldid=85620

Thank you! So ported crrev.com/c/802606.
Although the test coverage is not yet enough, could you take a quick grace to see if we can move forward?
If so, I'll add unittests for new code.

> The question was for comparison with the templated U type which is in the newer proposal.

Oh, I understood. Thank you for explanation!
Owner: hidehiko@chromium.org
Status: Started (was: Available)
Project Member

Comment 17 by bugdroid1@chromium.org, Dec 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25bbd5e7ab85c8c615ea9d2232549b01f932004f

commit 25bbd5e7ab85c8c615ea9d2232549b01f932004f
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Wed Dec 20 04:43:07 2017

Extract Base class from Optional.

This is preparation to catch up C++17 Optional spec.
To support conditional copy/move ctors/assign-operators,
inheriting trick is needed.
As its preparation, this CL extracts basic ctors and
copy/move assign-operators to a base class.
Also, Init(), InitOrAssign() and FreeIfneeded() are moved
to the base class because they need to implement
extracted methods.

This CL is pure refactoring, so Optional's API should have
no change.

BUG= 784732 
TEST=Trybot.

Change-Id: I197b573b1472d93c29ad594023f07097fa9b4510
Reviewed-on: https://chromium-review.googlesource.com/832586
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525264}
[modify] https://crrev.com/25bbd5e7ab85c8c615ea9d2232549b01f932004f/base/optional.h

Project Member

Comment 18 by bugdroid1@chromium.org, Dec 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a534c36c7505abeba7da78a91fbb235bd2c11c2

commit 7a534c36c7505abeba7da78a91fbb235bd2c11c2
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Wed Dec 20 10:56:26 2017

Update comparator operator definition of base::Optional.

To catch up c++17 definition, this CL updates the signature
and implementation of comparator operators.

BUG= 784732 
TEST=Ran base_unittests --gtest_filter=OptionalTest.

Change-Id: I4a21f8af1ec5ca0af9946250f3855f198077d52c
Reviewed-on: https://chromium-review.googlesource.com/832547
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525302}
[modify] https://crrev.com/7a534c36c7505abeba7da78a91fbb235bd2c11c2/base/optional.h
[modify] https://crrev.com/7a534c36c7505abeba7da78a91fbb235bd2c11c2/base/optional_unittest.cc
[modify] https://crrev.com/7a534c36c7505abeba7da78a91fbb235bd2c11c2/services/network/public/cpp/cors_error_status.h

Project Member

Comment 19 by bugdroid1@chromium.org, Jan 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5134ab38ad1ef0344106eea8e75c19df3f271954

commit 5134ab38ad1ef0344106eea8e75c19df3f271954
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Mon Jan 08 18:17:23 2018

Implement conditional constexpr copy-/move- constructors.

To catch up with C++17 spec, this implements conditional
constexpr-ness for copy-/move- constructors.

BUG= 784732 
TEST=Ran trybot.

Change-Id: I6a5ffdaf4dbe5ad5ff458494e07287d9e5fc0333
Reviewed-on: https://chromium-review.googlesource.com/839842
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527672}
[modify] https://crrev.com/5134ab38ad1ef0344106eea8e75c19df3f271954/base/optional.h
[modify] https://crrev.com/5134ab38ad1ef0344106eea8e75c19df3f271954/base/optional_unittest.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Jan 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1c8789c71dbdaeeef98ecd52c9715495824e6b0

commit f1c8789c71dbdaeeef98ecd52c9715495824e6b0
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Fri Jan 19 23:50:24 2018

Fix non-copyable class's optional move.

BUG= 784732 
TEST=Ran base_unittests -gtest_filter=*Optional*

Change-Id: Ibb5d7cc5d62deacdba7f811f5a7b83c1c58c3907
Reviewed-on: https://chromium-review.googlesource.com/855976
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530663}
[modify] https://crrev.com/f1c8789c71dbdaeeef98ecd52c9715495824e6b0/base/optional.h
[modify] https://crrev.com/f1c8789c71dbdaeeef98ecd52c9715495824e6b0/base/optional_unittest.cc

Project Member

Comment 21 by bugdroid1@chromium.org, Jan 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5cae9645215d02cb1f986a181a208f8a4817fc86

commit 5cae9645215d02cb1f986a181a208f8a4817fc86
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Fri Jan 26 18:01:11 2018

Implement conditional copy/move ctors/assign-operators.

BUG= 784732 
TEST=Ran trybot.

Change-Id: Iec5f9eaa7482d4e23f5bf2eea4b34c9cd867f89d
Reviewed-on: https://chromium-review.googlesource.com/856021
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532004}
[modify] https://crrev.com/5cae9645215d02cb1f986a181a208f8a4817fc86/base/optional.h
[modify] https://crrev.com/5cae9645215d02cb1f986a181a208f8a4817fc86/base/optional_unittest.cc

Project Member

Comment 22 by bugdroid1@chromium.org, Jan 31 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d39417a581d92178fea880cc5e4768e17db265f9

commit d39417a581d92178fea880cc5e4768e17db265f9
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Wed Jan 31 03:22:42 2018

Implement converting constructors from Optional<U>.

BUG= 784732 
TEST=Ran trybot.

Change-Id: Icbb9c5596b045ca0c684cdad0343a2605d00dbb1
Reviewed-on: https://chromium-review.googlesource.com/856296
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533185}
[modify] https://crrev.com/d39417a581d92178fea880cc5e4768e17db265f9/base/BUILD.gn
[modify] https://crrev.com/d39417a581d92178fea880cc5e4768e17db265f9/base/optional.h
[modify] https://crrev.com/d39417a581d92178fea880cc5e4768e17db265f9/base/optional_unittest.cc
[add] https://crrev.com/d39417a581d92178fea880cc5e4768e17db265f9/base/optional_unittest.nc

Comment 23 by carew...@gmail.com, Jan 31 2018

This change made Chromium 65 depend on C++17. Was that intentional?

Comment 24 by carew...@gmail.com, Jan 31 2018

If the follow-up changes adresses that, perhaps needs to be merged into M65.
> This change made Chromium 65 depend on C++17. Was that intentional?

No, it's not. We don't compile chromium with c++17 enabled on any toolchain. Can you point out what is problematic?

Comment 26 Deleted

The recent changes to base::Optional included in this bug have broken GCC build (at least with GCC 7.3.0). Not sure how to fix it, or if the pending patches in the review chain will fix this.

The problem seems that it is still trying to access new() constructor for mojo StructPtr, that has been inhibited by DISALLOW_COPY_AND_ASSIGN.

| In file included from .../build/tmp/work/cortexa7hf-neon-vfpv4-poky-linux-gnueabi/chromium-x11-dev/66.0.3346.8-r0/recipe-sysroot/usr/include/c++/7.3.0/memory:64:0,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/system/buffer.h:17,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/system/core.h:8,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/lib/bindings_internal.h:16,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/lib/array_internal.h:18,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/array_data_view.h:10,
|                  from gen/chrome/browser/engagement/site_engagement_details.mojom-shared.h:16,
|                  from gen/chrome/browser/engagement/site_engagement_details.mojom-shared.cc:10:
| /home/dape/Development/rpi/poky-pyro/build/tmp/work/cortexa7hf-neon-vfpv4-poky-linux-gnueabi/chromium-x11-dev/66.0.3346.8-r0/recipe-sysroot/usr/include/c++/7.3.0/bits/stl_construct.h: In instantiation of 'void std::_Construct(_T1*, _Args&& ...) [with _T1 = mojo::StructPtr<mojo::native::SerializedHandle>; _Args = {const mojo::StructPtr<mojo::native::SerializedHandle>&}]':
| /home/dape/Development/rpi/poky-pyro/build/tmp/work/cortexa7hf-neon-vfpv4-poky-linux-gnueabi/chromium-x11-dev/66.0.3346.8-r0/recipe-sysroot/usr/include/c++/7.3.0/bits/stl_uninitialized.h:83:18:   required from 'static _ForwardIterator std::__uninitialized_copy<_TrivialValueTypes>::__uninit_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = __gnu_cxx::__normal_iterator<const mojo::StructPtr<mojo::native::SerializedHandle>*, std::vector<mojo::StructPtr<mojo::native::SerializedHandle> > >; _ForwardIterator = mojo::StructPtr<mojo::native::SerializedHandle>*; bool _TrivialValueTypes = false]'
| /home/dape/Development/rpi/poky-pyro/build/tmp/work/cortexa7hf-neon-vfpv4-poky-linux-gnueabi/chromium-x11-dev/66.0.3346.8-r0/recipe-sysroot/usr/include/c++/7.3.0/bits/stl_uninitialized.h:134:15:   required from '_ForwardIterator std::uninitialized_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = __gnu_cxx::__normal_iterator<const mojo::StructPtr<mojo::native::SerializedHandle>*, std::vector<mojo::StructPtr<mojo::native::SerializedHandle> > >; _ForwardIterator = mojo::StructPtr<mojo::native::SerializedHandle>*]'
| /home/dape/Development/rpi/poky-pyro/build/tmp/work/cortexa7hf-neon-vfpv4-poky-linux-gnueabi/chromium-x11-dev/66.0.3346.8-r0/recipe-sysroot/usr/include/c++/7.3.0/bits/stl_uninitialized.h:289:37:   required from '_ForwardIterator std::__uninitialized_copy_a(_InputIterator, _InputIterator, _ForwardIterator, std::allocator<_Tp>&) [with _InputIterator = __gnu_cxx::__normal_iterator<const mojo::StructPtr<mojo::native::SerializedHandle>*, std::vector<mojo::StructPtr<mojo::native::SerializedHandle> > >; _ForwardIterator = mojo::StructPtr<mojo::native::SerializedHandle>*; _Tp = mojo::StructPtr<mojo::native::SerializedHandle>]'
| /home/dape/Development/rpi/poky-pyro/build/tmp/work/cortexa7hf-neon-vfpv4-poky-linux-gnueabi/chromium-x11-dev/66.0.3346.8-r0/recipe-sysroot/usr/include/c++/7.3.0/bits/stl_vector.h:331:31:   required from 'std::vector<_Tp, _Alloc>::vector(const std::vector<_Tp, _Alloc>&) [with _Tp = mojo::StructPtr<mojo::native::SerializedHandle>; _Alloc = std::allocator<mojo::StructPtr<mojo::native::SerializedHandle> >]'
| /home/dape/Development/rpi/poky-pyro/build/tmp/work/cortexa7hf-neon-vfpv4-poky-linux-gnueabi/chromium-x11-dev/66.0.3346.8-r0/recipe-sysroot/usr/include/c++/7.3.0/type_traits:1406:12:   required from 'struct std::is_trivially_copy_constructible<std::vector<mojo::StructPtr<mojo::native::SerializedHandle> > >'
| ../../../chromium-66.0.3346.8/base/optional.h:287:22:   required from 'class base::internal::OptionalBase<std::vector<mojo::StructPtr<mojo::native::SerializedHandle> > >'
| ../../../chromium-66.0.3346.8/base/optional.h:370:7:   required from 'class base::Optional<std::vector<mojo::StructPtr<mojo::native::SerializedHandle> > >'
| gen/mojo/public/interfaces/bindings/native_struct.mojom.h:261:52:   required from here
| /home/dape/Development/rpi/poky-pyro/build/tmp/work/cortexa7hf-neon-vfpv4-poky-linux-gnueabi/chromium-x11-dev/66.0.3346.8-r0/recipe-sysroot/usr/include/c++/7.3.0/bits/stl_construct.h:75:7: error: 'mojo::StructPtr<S>::StructPtr(const mojo::StructPtr<S>&) [with S = mojo::native::SerializedHandle]' is private within this context
|      { ::new(static_cast<void*>(__p)) _T1(std::forward<_Args>(__args)...); }
|        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| In file included from ../../../chromium-66.0.3346.8/base/logging.h:21:0,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/lib/array_internal.h:15,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/array_data_view.h:10,
|                  from gen/chrome/browser/engagement/site_engagement_details.mojom-shared.h:16,
|                  from gen/chrome/browser/engagement/site_engagement_details.mojom-shared.cc:10:
| ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/struct_ptr.h:110:28: note: declared private here
|    DISALLOW_COPY_AND_ASSIGN(StructPtr);
|                             ^
| ../../../chromium-66.0.3346.8/base/macros.h:27:3: note: in definition of macro 'DISALLOW_COPY'
|    TypeName(const TypeName&) = delete
|    ^~~~~~~~
| ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/struct_ptr.h:110:3: note: in expansion of macro 'DISALLOW_COPY_AND_ASSIGN'
|    DISALLOW_COPY_AND_ASSIGN(StructPtr);
|    ^~~~~~~~~~~~~~~~~~~~~~~~
| In file included from /home/dape/Development/rpi/poky-pyro/build/tmp/work/cortexa7hf-neon-vfpv4-poky-linux-gnueabi/chromium-x11-dev/66.0.3346.8-r0/recipe-sysroot/usr/include/c++/7.3.0/memory:64:0,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/system/buffer.h:17,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/system/core.h:8,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/lib/bindings_internal.h:16,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/lib/array_internal.h:18,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/array_data_view.h:10,
|                  from gen/chrome/browser/engagement/site_engagement_details.mojom-shared.h:16,
|                  from gen/chrome/browser/engagement/site_engagement_details.mojom-shared.cc:10:
| /home/dape/Development/rpi/poky-pyro/build/tmp/work/cortexa7hf-neon-vfpv4-poky-linux-gnueabi/chromium-x11-dev/66.0.3346.8-r0/recipe-sysroot/usr/include/c++/7.3.0/bits/stl_construct.h:75:7: error: use of deleted function 'mojo::StructPtr<S>::StructPtr(const mojo::StructPtr<S>&) [with S = mojo::native::SerializedHandle]'
|      { ::new(static_cast<void*>(__p)) _T1(std::forward<_Args>(__args)...); }
|        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| In file included from ../../../chromium-66.0.3346.8/base/logging.h:21:0,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/lib/array_internal.h:15,
|                  from ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/array_data_view.h:10,
|                  from gen/chrome/browser/engagement/site_engagement_details.mojom-shared.h:16,
|                  from gen/chrome/browser/engagement/site_engagement_details.mojom-shared.cc:10:
| ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/struct_ptr.h:110:28: note: declared here
|    DISALLOW_COPY_AND_ASSIGN(StructPtr);
|                             ^
| ../../../chromium-66.0.3346.8/base/macros.h:27:3: note: in definition of macro 'DISALLOW_COPY'
|    TypeName(const TypeName&) = delete
|    ^~~~~~~~
| ../../../chromium-66.0.3346.8/mojo/public/cpp/bindings/struct_ptr.h:110:3: note: in expansion of macro 'DISALLOW_COPY_AND_ASSIGN'
|    DISALLOW_COPY_AND_ASSIGN(StructPtr);
|    ^~~~~~~~~~~~~~~~~~~~~~~~

Could you bisect to the exact CL?
So, according to the error log,
https://chromium-review.googlesource.com/c/chromium/src/+/839842/5/base/optional.h seems to hit a compiler issue: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80654

Small repro is;

class NoCopy {
 public:
  NoCopy(const NoCopy&) = delete;
};

std::is_trivially_copy_constructible<std::vector<NoCopy>>::value;

I can fix this specific case.
However, the root cause looks like is_trivially_copy_constructible's change, and it seems like
compiler support is needed to fix the root cause, rather than just a library change.
danakj@, do you think the point fix is ok in this kind of case?

What would it look like?
Cc: tzik@chromium.org
Re #30: here it is https://chromium-review.googlesource.com/c/chromium/src/+/927942 (thanks, tzik@, for helping test!)

TBH, I'm not very sure if we should put this in base, as std::optional in g++7 has same issue. IIUC, in g++8 (ToT), the problem is already fixed.

Project Member

Comment 32 by bugdroid1@chromium.org, Feb 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/14f92bee690d0b1884864db08f3e7f03abc85c00

commit 14f92bee690d0b1884864db08f3e7f03abc85c00
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Wed Feb 21 06:01:28 2018

Implement value forward constructor.

So, older constructors taking const T& or T&& is removed.

BUG= 784732 
TEST=Ran trybot.

Change-Id: I806b1880bf3bd2bd25da764f7592299a1a742366
Reviewed-on: https://chromium-review.googlesource.com/856380
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538032}
[modify] https://crrev.com/14f92bee690d0b1884864db08f3e7f03abc85c00/base/optional.h
[modify] https://crrev.com/14f92bee690d0b1884864db08f3e7f03abc85c00/base/optional_unittest.cc
[modify] https://crrev.com/14f92bee690d0b1884864db08f3e7f03abc85c00/base/optional_unittest.nc

Project Member

Comment 33 by bugdroid1@chromium.org, Feb 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40ee4aee34e39b711f266bbe3e03f4c033b9cf71

commit 40ee4aee34e39b711f266bbe3e03f4c033b9cf71
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Fri Feb 23 04:24:12 2018

Update (non-copy, non-move) assign operators.

Fix perfect-forwarded assign operator to look at condition to
decide whether it should participate in overload resolution.

Add Optional<U> copy- and move-like assign operators.
For that implementation, OptionalBase's copy-/move-assign
operators are slightly refactored.

BUG= 784732 
TEST=Ran trybot.

Change-Id: I69db9def857a1cce8e7b05f0c6e11922ee8d95db
Reviewed-on: https://chromium-review.googlesource.com/856539
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538699}
[modify] https://crrev.com/40ee4aee34e39b711f266bbe3e03f4c033b9cf71/base/optional.h
[modify] https://crrev.com/40ee4aee34e39b711f266bbe3e03f4c033b9cf71/base/optional_unittest.cc

Project Member

Comment 34 by bugdroid1@chromium.org, Feb 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b467afef9550281b7c8763c765735c9e9b4d3f0d

commit b467afef9550281b7c8763c765735c9e9b4d3f0d
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Fri Feb 23 07:01:30 2018

Update smaller parts of Optional to catch up with C++17 spec.

This has several update parts.
- Remove DCHECK from operator->(), as redirected value() has same check.
- rvalue value_or is marked as constexpr.
- emplace now returns T&.
- Adds make_optional(Args&&... args) overloading.
- swap is now conditionally defined.

BUG= 784732 
TEST=Ran trybot.

Change-Id: Ife12c56374f14fe3514aeee4f161c9bafce5c135
Reviewed-on: https://chromium-review.googlesource.com/857357
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538717}
[modify] https://crrev.com/b467afef9550281b7c8763c765735c9e9b4d3f0d/base/optional.h
[modify] https://crrev.com/b467afef9550281b7c8763c765735c9e9b4d3f0d/base/optional_unittest.cc

Project Member

Comment 35 by bugdroid1@chromium.org, Feb 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d8468a07f374c11425494271256151fb6fe0c34

commit 4d8468a07f374c11425494271256151fb6fe0c34
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Fri Feb 23 09:50:41 2018

Workaround for g++7 is_trivially_copy_constructible failure.

cf) https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80654
Please see also  crbug.com/784732 #27 for details.

BUG= 784732 
TEST=Some with GCC.

Change-Id: I0a6d28d9c26ac9ed026d137e17fddbe86586f1e1
Reviewed-on: https://chromium-review.googlesource.com/927942
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538740}
[modify] https://crrev.com/4d8468a07f374c11425494271256151fb6fe0c34/base/optional.h
[modify] https://crrev.com/4d8468a07f374c11425494271256151fb6fe0c34/base/template_util.h
[modify] https://crrev.com/4d8468a07f374c11425494271256151fb6fe0c34/base/template_util_unittest.cc

Project Member

Comment 36 by bugdroid1@chromium.org, Feb 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3aa61df502c16c7231afb2bcef4e94ff30a16b0b

commit 3aa61df502c16c7231afb2bcef4e94ff30a16b0b
Author: Hidehiko Abe <hidehiko@chromium.org>
Date: Sat Feb 24 12:47:07 2018

Mark noexcept for Optional to catch up with C++17 spec.

Although, exceptions are not used in chromium,
some standard libraries depend on noexcept markers.
Because Optional is an alternative of standard library,
it is nice to have such markers.

BUG= 784732 
TEST=Ran trybot.

Change-Id: I958bc3f18e1d898d65cc106748795c6a27462650
Reviewed-on: https://chromium-review.googlesource.com/857358
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539012}
[modify] https://crrev.com/3aa61df502c16c7231afb2bcef4e94ff30a16b0b/base/optional.h
[modify] https://crrev.com/3aa61df502c16c7231afb2bcef4e94ff30a16b0b/base/optional_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment