base::Optional not updated to the latest version in the standard |
|||||||
Issue descriptionhttp://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.
,
Nov 14 2017
,
Nov 14 2017
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?
,
Nov 14 2017
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
,
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.
,
Nov 14 2017
,
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.
,
Nov 14 2017
,
Nov 15 2017
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.
,
Nov 15 2017
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.
,
Nov 15 2017
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.
,
Nov 16 2017
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.
,
Nov 21 2017
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.
,
Nov 21 2017
> 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.
,
Dec 1 2017
>> 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!
,
Dec 18 2017
,
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
,
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
,
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
,
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
,
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
,
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
,
Jan 31 2018
This change made Chromium 65 depend on C++17. Was that intentional?
,
Jan 31 2018
If the follow-up changes adresses that, perhaps needs to be merged into M65.
,
Jan 31 2018
> 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?
,
Feb 20 2018
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);
| ^~~~~~~~~~~~~~~~~~~~~~~~
,
Feb 20 2018
Could you bisect to the exact CL?
,
Feb 20 2018
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?
,
Feb 20 2018
What would it look like?
,
Feb 21 2018
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.
,
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
,
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
,
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
,
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
,
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
,
Feb 26 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by danakj@chromium.org
, Nov 14 2017