Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1245163002:
Base: add Optional<T>. (Closed)
|
Created:
5 years, 5 months ago by mlamouri (slow - plz ping) Modified:
4 years, 8 months ago CC:
chromium-reviews, dcheng, Sami Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBase: add Optional<T>.
This is an implementation of the C++17 std::optional<> feature:
http://en.cppreference.com/w/cpp/utility/optional
Chromium documentation:
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md
BUG=521269
Committed: https://crrev.com/8756e4f20df53bbb7fbb49b2ca19fda93a95bdb4
Cr-Commit-Position: refs/heads/master@{#388187}
Committed: https://crrev.com/53f6b258355ce36752ea7a8dcf8953560b5be818
Cr-Commit-Position: refs/heads/master@{#388232}
Patch Set 1 #Patch Set 2 : format #Patch Set 3 : add files #Patch Set 4 : Nullable -> Optional #
Total comments: 11
Patch Set 5 : #
Total comments: 21
Patch Set 6 : closer to spec implementation #
Total comments: 69
Patch Set 7 : review comments #
Total comments: 21
Patch Set 8 : review comments #Patch Set 9 : dcheng's suggestion #Patch Set 10 : attempt to fix windows build #Patch Set 11 : documentation #Patch Set 12 : missed one expect_true #Patch Set 13 : moar test changes for msvc #
Total comments: 4
Patch Set 14 : updated documentation #
Total comments: 1
Patch Set 15 : doc changes #Patch Set 16 : win7 fix #Patch Set 17 : fix md #
Messages
Total messages: 98 (32 generated)
mlamouri@chromium.org changed reviewers: + thakis@chromium.org
There's no motivation anywhere why this is useful – nothing in the cl description, no bug link, no discussion thread, no class comment. So I'll guess that the motivation is that "more annotations can't hurt" – but I think the signal / line noise ratio for nullable annotations isn't all that great. If we do want something like this, a NonNull is probably more interesting, and using something based on http://clang.llvm.org/docs/AttributeReference.html#nullability-attributes is probably a better approach these days. But even with that I don't think this is super useful. (Case in point: This is only used pretty sparsely in blink too)
On 2015/07/21 15:01:10, Nico wrote: > There's no motivation anywhere why this is useful – nothing in the cl > description, no bug link, no discussion thread, no class comment. > > So I'll guess that the motivation is that "more annotations can't hurt" – but I > think the signal / line noise ratio for nullable annotations isn't all that > great. If we do want something like this, a NonNull is probably more > interesting, and using something based on > http://clang.llvm.org/docs/AttributeReference.html#nullability-attributes is > probably a better approach these days. But even with that I don't think this is > super useful. (Case in point: This is only used pretty sparsely in blink too) (Note that this is not about pointers being null, but about optional types. The latest place where this came up was https://codereview.chromium.org/1234653004/.)
Sorry, for the very limited context. I had to run out of the office unexpectedly. The idea here is that for content::Manifest, we might have fields being undefined and depending on the type we can or can't have a value for that. As a result, we have a uint32_t set as a int64_t in order to have a simple way to check if it is undefined (-1). For strings, we can use NullableString16 which has proven to be really nice. The solution we have for uint32_t could also be solved with having a boolean on the side (but that's really Nullable<>) or using a pointer (but that isn't great and has a bigger memory footprint than a boolean). Nullable<uint32_t> would have been the ideal solution to our problem. I think that's what Bernhard, Lalit and I felt. So I wondered that we maybe could try to introduce it in Chromium so we could use it.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245163002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
lalitm@google.com changed reviewers: + lalitm@google.com
thakis: could you please consider mlamouri's explanation. It would be really good to have this included especially for the use case in my CL which bauerb links.
lalitm@google.com changed reviewers: + danakj@chromium.org
+ danakj because dmurph sitting next to me said so. Sorry danakj -- dmurph
On 2015/07/21 19:14:22, Mounir Lamouri OOO up to 3 Aug wrote: > Sorry, for the very limited context. I had to run out of the office > unexpectedly. > > The idea here is that for content::Manifest, we might have fields being > undefined and depending on the type we can or can't have a value for that. As a > result, we have a uint32_t set as a int64_t in order to have a simple way to > check if it is undefined (-1). > > For strings, we can use NullableString16 which has proven to be really nice. The > solution we have for uint32_t could also be solved with having a boolean on the > side (but that's really Nullable<>) or using a pointer (but that isn't great and > has a bigger memory footprint than a boolean). > > Nullable<uint32_t> would have been the ideal solution to our problem. I think > that's what Bernhard, Lalit and I felt. So I wondered that we maybe could try to > introduce it in Chromium so we could use it. Why isn't this "Optional" based on the std::experimental::optional http://en.cppreference.com/w/cpp/experimental/optional and similar to blink's https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Is there some difference? There's still no BUG= or motivation in the CL description, digging through CL comments isn't the right way to find it.
I've created a bug. Regarding the difference between Optional<> and Nullable<>, I guess the gist is the same but Blink's implementations are quite different with Optional<> using the heap. Regarding C++'s optional<>, I'm a bit worried that having implicit bool conversion could be a problem. If you think that's fine, I would gladly rename Nullable<> to Optional<> and implement an interface closer to the C++ one (remove is_null() and change get() to ::value()). Would you prefer the implementation to look more like Blink's Nullable<> or Optional<>?
On 2015/08/15 13:55:52, Mounir Lamouri wrote: > I've created a bug. > > Regarding the difference between Optional<> and Nullable<>, I guess the gist is > the same but Blink's implementations are quite different with Optional<> using > the heap. > > Regarding C++'s optional<>, I'm a bit worried that having implicit bool > conversion could be a problem. If you think that's fine, I would gladly rename > Nullable<> to Optional<> and implement an interface closer to the C++ one > (remove is_null() and change get() to ::value()). Would you prefer the > implementation to look more like Blink's Nullable<> or Optional<>? AFAIK, implicit conversions to bool are fine; we have that for example for scoped_ptr.
I have updated the CL to use Optional<T> instead of Nullable<T>. It also uses a subset of the experimental::optional interface. PTAL
https://codereview.chromium.org/1245163002/diff/60001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/60001/base/optional.h#newcode17 base/optional.h:17: // subset of it. It uses nullptr_t instead of exeperimental::nullopt_t. If preferred, I can create an Optional::nullopt_t?
So I think that I would like to move WTF::Optional to base, but this implementation as it is differs from libc++ in behaviour and that's not great. https://codereview.chromium.org/1245163002/diff/60001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/60001/base/optional.h#newcode16 base/optional.h:16: // It is not meant to be a full implementation of the experimental class but a Why? Can you expand? https://codereview.chromium.org/1245163002/diff/60001/base/optional.h#newcode17 base/optional.h:17: // subset of it. It uses nullptr_t instead of exeperimental::nullopt_t. On 2015/08/23 14:35:03, Mounir Lamouri wrote: > If preferred, I can create an Optional::nullopt_t? nullptr i guess is misleading cuz this isn't holding a pointer. also it could be holding a pointer, and nullopt would be different from nullptr. ie missing vs present-but-null. base::nullopt_t is probably what we would want? https://codereview.chromium.org/1245163002/diff/60001/base/optional.h#newcode22 base/optional.h:22: Optional(std::nullptr_t) : value_(), is_null_(true) { } std::nullptr_t is a c++11 library feature. no go. you could use decltype(nullptr) https://codereview.chromium.org/1245163002/diff/60001/base/optional.h#newcode28 base/optional.h:28: Optional& operator=(std::nullptr_t) { styleguide says to name all arguments even if not used https://codereview.chromium.org/1245163002/diff/60001/base/optional.h#newcode70 base/optional.h:70: T value_; This is going to default-construct a T even if you don't have one in the optional. But not all Ts are default-constructable. You should take a look at the WTF implementation which does not have this unintended (and different from libc++) behaviour: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... libc++ uses a union, but that depends on union of class types which is a new c++11 feature: http://en.cppreference.com/w/cpp/language/union and not allowed by our c++11 style guide yet (no one has proposed it afaik). jbroman says he tried it and found an issue with msvc maybe.
Description was changed from ========== Base: add Optional<T>. Inspired from C++ experimental::optional<>. BUG=521269 ========== to ========== Base: add Optional<T>. It is an implementation of C++ experimental::optional<>. BUG=521269 ==========
mlamouri@chromium.org changed reviewers: - lalitm@google.com
danakj@, I've applied your comments. The implementation is closer to the C++ standard. I've listed the difference in the file. PTAL. https://codereview.chromium.org/1245163002/diff/60001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/60001/base/optional.h#newcode16 base/optional.h:16: // It is not meant to be a full implementation of the experimental class but a On 2015/10/15 at 20:45:26, danakj wrote: > Why? Can you expand? Done. https://codereview.chromium.org/1245163002/diff/60001/base/optional.h#newcode17 base/optional.h:17: // subset of it. It uses nullptr_t instead of exeperimental::nullopt_t. On 2015/10/15 at 20:45:26, danakj wrote: > On 2015/08/23 14:35:03, Mounir Lamouri wrote: > > If preferred, I can create an Optional::nullopt_t? > > nullptr i guess is misleading cuz this isn't holding a pointer. also it could be holding a pointer, and nullopt would be different from nullptr. ie missing vs present-but-null. base::nullopt_t is probably what we would want? Done. https://codereview.chromium.org/1245163002/diff/60001/base/optional.h#newcode22 base/optional.h:22: Optional(std::nullptr_t) : value_(), is_null_(true) { } On 2015/10/15 at 20:45:26, danakj wrote: > std::nullptr_t is a c++11 library feature. no go. > > you could use decltype(nullptr) I switched to base::nullopt_t; https://codereview.chromium.org/1245163002/diff/60001/base/optional.h#newcode28 base/optional.h:28: Optional& operator=(std::nullptr_t) { On 2015/10/15 at 20:45:26, danakj wrote: > styleguide says to name all arguments even if not used Done. https://codereview.chromium.org/1245163002/diff/60001/base/optional.h#newcode70 base/optional.h:70: T value_; On 2015/10/15 at 20:45:26, danakj wrote: > This is going to default-construct a T even if you don't have one in the optional. But not all Ts are default-constructable. > > You should take a look at the WTF implementation which does not have this unintended (and different from libc++) behaviour: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > libc++ uses a union, but that depends on union of class types which is a new c++11 feature: http://en.cppreference.com/w/cpp/language/union and not allowed by our c++11 style guide yet (no one has proposed it afaik). jbroman says he tried it and found an issue with msvc maybe. Done.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245163002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245163002/80001
The CQ bit was unchecked by commit-bot@chromium.org
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@, PTAL. I will investigate the Windows build if the current CL is more in line with what you were expecting.
This looks really great, thanks! We're really close to allowing && so I think I'd prefer holding off for that so we don't introduce an API just to change it. At that point this seems like it'll be great to add and we can delete the WTF one. https://codereview.chromium.org/1245163002/diff/80001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode16 base/optional.h:16: explicit nullopt_t(int _) { } i recently discovered the style guide now says you can omit names of unused variables, so no _ needed https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode26 base/optional.h:26: // - All methods using rvalue reference (ie. && operator). I think that I'd like to wait on this until we allow doing &&, which shouldn't be long. We should be able to implement everything at that point I think?
*insert a bunch of comments about lets make this match the experimental one exactly* https://codereview.chromium.org/1245163002/diff/80001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode39 base/optional.h:39: Optional(const Optional& other) Can you add a Optional(Optional&&) constructor? https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode45 base/optional.h:45: Optional(const T& value) And an Optional(T&&) constructor https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode61 base/optional.h:61: Optional& operator=(const Optional& other) { can you add operator=(Optional&&) https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode75 base/optional.h:75: Optional& operator=(const T& value) { Let's make this take a T&& https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode98 base/optional.h:98: explicit operator bool() const { return !is_null_; } explicit operator is banned: http://chromium-cpp.appspot.com/#core-blacklist you'll have to do a testable type trick instead (see scoped_ptr for example) https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode100 base/optional.h:100: T& value() { can you ref-quality this with & https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode105 base/optional.h:105: const T& value() const { and this with &, and add the T&& versions of value() and operator* from http://en.cppreference.com/w/cpp/experimental/optional? https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode111 base/optional.h:111: T value_or(U&& default_value) const { can add the non-const version of this method too
Is this going to land any time soon? I was looking for something like this to exist so I recommend using it here: https://codereview.chromium.org/1666583002
I need to apply the last comments that Dana left. I hope to find some time next week (famous last words...).
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
Also adding my vote of support - this is exactly what I need in http://crrev.com/1713723002
Dana, can you PTAL. Could you check the bool operator. I couldn't get the Testable hack to work. Also, what's the latest way to handle hash in Chromium? https://codereview.chromium.org/1245163002/diff/80001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode16 base/optional.h:16: explicit nullopt_t(int _) { } On 2015/11/19 at 20:55:46, danakj_OOO_back_2_22 wrote: > i recently discovered the style guide now says you can omit names of unused variables, so no _ needed Done. https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode26 base/optional.h:26: // - All methods using rvalue reference (ie. && operator). On 2015/11/19 at 20:55:46, danakj wrote: > I think that I'd like to wait on this until we allow doing &&, which shouldn't be long. We should be able to implement everything at that point I think? Kind of. I've updated the list. Let me know how that sounds. https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode39 base/optional.h:39: Optional(const Optional& other) On 2015/12/10 at 23:45:45, danakj wrote: > Can you add a Optional(Optional&&) constructor? Done. https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode45 base/optional.h:45: Optional(const T& value) On 2015/12/10 at 23:45:46, danakj wrote: > And an Optional(T&&) constructor Done. https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode51 base/optional.h:51: // implementation doesn't use std::is_trivially_destructible. Is it okay to use std::is_trivially_destructible? https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode61 base/optional.h:61: Optional& operator=(const Optional& other) { On 2015/12/10 at 23:45:45, danakj wrote: > can you add operator=(Optional&&) Done. https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode75 base/optional.h:75: Optional& operator=(const T& value) { On 2015/12/10 at 23:45:45, danakj wrote: > Let's make this take a T&& Done. I've included the per-spec std::enable_if. https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode98 base/optional.h:98: explicit operator bool() const { return !is_null_; } On 2015/12/10 at 23:45:46, danakj wrote: > explicit operator is banned: http://chromium-cpp.appspot.com/#core-blacklist > > you'll have to do a testable type trick instead (see scoped_ptr for example) I couldn't get that to work. The issue is that I can't access the raw data directly (ie. buffer_.data_). I'm sure there is some C++ magic I'm missing. https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode100 base/optional.h:100: T& value() { On 2015/12/10 at 23:45:45, danakj wrote: > can you ref-quality this with & Done. https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode105 base/optional.h:105: const T& value() const { On 2015/12/10 at 23:45:45, danakj wrote: > and this with &, > > and add the T&& versions of value() and operator* from http://en.cppreference.com/w/cpp/experimental/optional? Done. https://codereview.chromium.org/1245163002/diff/80001/base/optional.h#newcode111 base/optional.h:111: T value_or(U&& default_value) const { On 2015/12/10 at 23:45:46, danakj wrote: > can add the non-const version of this method too Done.
https://codereview.chromium.org/1245163002/diff/100001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode20 base/optional.h:20: explicit nullopt_t(int) { } nit: no space in {}. did you git cl format? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode20 base/optional.h:20: explicit nullopt_t(int) { } Can you throw a TODO on here that it should be constexptr once MSVC 2015? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode25 base/optional.h:25: const in_place_t in_place{}; this is brace-initialization which we don't allow yet. you can just write this as below? const in_place_t in_place; Also, TODO for constexpr? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode29 base/optional.h:29: const nullopt_t nullopt(0); TODO for constexpr here too? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode32 base/optional.h:32: // class: http://en.cppreference.com/w/cpp/experimental/optional/optional Link should be http://en.cppreference.com/w/cpp/experimental/optional (you're linking to constructor) https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode38 base/optional.h:38: // - Private member |val| (for exposition) is not implemented. The |val| is just an example, I don't think you need to mention it here. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode39 base/optional.h:39: // - The destructor doesn't use std::std::is_trivially_destructible. can you explain why? msvc problems? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode40 base/optional.h:40: // - All the non-members are in the 'base' namespace instead of `std'.` quotes got a little weird around "std" https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode45 base/optional.h:45: Optional(base::nullopt_t opt) : Optional() { } nit: you could leave the parameter unnamed https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode54 base/optional.h:54: init(std::move(*other.buffer_.template data_as<T>())); why not std::move(other.value()) ? it should cast the T& to a T&& to do what you want? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode74 base/optional.h:74: Optional& operator=(base::nullopt_t opt) { nit: can leave the parameter unnamed https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode95 base/optional.h:95: init_or_assign(std::move(*other.buffer_.template data_as<T>())); why not std::move(other.value()) ? it should cast the T& to a T&& to do what you want? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:102: <std::is_same<std::decay<U>, T>::value, Optional&>::type The formatting here.. is that clang-format? Can you write this with the enable_if on the left side of the function name (instead of using auto -> syntax)? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:110: return buffer_.template data_as<T>(); return &value()? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:115: return buffer_.template data_as<T>(); return &value()? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:127: return std::forward<T>(value()); why forward? just std::move(value())? optional won't compile if T is a reference type for many reasons. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:131: return std::forward<T>(value()); std::move(value())? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:148: typedef T* Optional::*Testable; prefer "using" to "typedef" https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:148: typedef T* Optional::*Testable; using Optional::*Testable = bool? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:153: return is_null_ ? nullptr : &Optional::buffer_.template data_as<T>(); return is_null_ ? nullptr : &Optional::is_null_ ? So.. I have good news and bad news. Bad news: Since this class uses ref qualifiers, it won't compile in MSVC 2013. So we'll have to wait for 2015. Good news: 2015 continues to be very soon, I heard say this week again? Next week? idk Better news: That means you can just use explicit operator, cuz that will work in MSVC 2015 too. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:178: T value_or(U&& default_value) const { this should be qualified as const& https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:178: T value_or(U&& default_value) const { can you add some static asserts here: // TODO(danakj): Enable this when MSVC 2015 happens. // static_assert(std::is_copy_constructible<T>::value, "T must be copy constructible"); static_assert(std::is_convertible<U, T>::value, "U must be convertible to T"); i'm assuming the 2nd one would work now.. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:179: return is_null_ ? default_value : value(); i think in the null case you need to actually return static_cast<T>(std::forward<U>(default_value)) https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:183: T value_or(U&& default_value) { this should be qualified as && https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:183: T value_or(U&& default_value) { can you add some static asserts here: // TODO(danakj): Enable this when MSVC 2015 happens. // static_assert(std::is_move_constructible<T>::value, "T must be move constructible"); static_assert(std::is_convertible<U, T>::value, "U must be convertible to T"); i'm assuming the 2nd one would work now.. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:184: return is_null_ ? default_value : value(); in null case: return static_cast<T>(std::forward<U>(default_value)) in non-null case: return std::move(value()) https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:204: swap(**this, *other); I think this has to be a typo in cppreference? It should be swap(value(), other.value())? The implementation in libcxx seems to agree. Is there a unit test case for this? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:210: init(T(std::forward<Args>(args)...)); This isn't quite the same thing as constructing from the arguments, this is constructing and then moving. You want to forward the args to init() (you'll need another init for this or skip using init() here) https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:214: void init(const T& value) { can you name these private methods in CamelCase style? They aren't necessarily that cheap. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:222: new (buffer_.template data_as<T>()) T(std::forward<T>(value)); just move() here, not forward. the function is not separately templated, so it will always be an rvalue ref. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:235: init(std::forward<T>(value)); just std::move() here https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:237: *buffer_.template data_as<T>() = std::forward<T>(value); and here https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:253: if (bool(lhs) != bool(rhs)) These are cast-styles that the google style guide doesn't like. Instead you can write static_cast<bool>(lhs) or !!lhs or lhs != nullopt. using nullopt is the most verbose but may also be the most clear, especially in cases where you are negating the bool cast. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:329: return bool(opt); can you write the > operators here and below in terms of the < operators above? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:394: return bool(opt) ? *opt < value : true; same here, write these in terms of the operator < and friends? https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:417: // TODO: implement hash function. put a name or a bug on it
Looks like VS 2015 has stuck now.
On 2016/03/14 at 18:59:49, danakj wrote: > Looks like VS 2015 has stuck now. I actually applied most of your comments last week but didn't finish yet. I know this is taking quite longer than expected. Thank you for your patience :)
On Mon, Mar 14, 2016 at 12:02 PM, <mlamouri@chromium.org> wrote: > On 2016/03/14 at 18:59:49, danakj wrote: > > Looks like VS 2015 has stuck now. > > I actually applied most of your comments last week but didn't finish yet. > I know > this is taking quite longer than expected. Thank you for your patience :) > No worries, we've been hampered by infrastructure anyhow. Thanks for keeping at it :) > https://codereview.chromium.org/1245163002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Base: add Optional<T>. It is an implementation of C++ experimental::optional<>. BUG=521269 ========== to ========== Base: add Optional<T>. It is an implementation of C++ experimental::optional<>. BUG=521269 ==========
Could you please let me know when this CL will be submitted? This CL is blocking my CL(https://codereview.chromium.org/1731153005/#msg29) now.
On 2016/03/17 07:23:58, horo wrote: > Could you please let me know when this CL will be submitted? > This CL is blocking my CL(https://codereview.chromium.org/1731153005/#msg29) > now. I added you to the cc list.
Description was changed from ========== Base: add Optional<T>. It is an implementation of C++ experimental::optional<>. BUG=521269 ========== to ========== Base: add Optional<T>. This is an implementation of the C++17 std::optional<> feature: http://en.cppreference.com/w/cpp/utility/optional/swap BUG=521269 ==========
Description was changed from ========== Base: add Optional<T>. This is an implementation of the C++17 std::optional<> feature: http://en.cppreference.com/w/cpp/utility/optional/swap BUG=521269 ========== to ========== Base: add Optional<T>. This is an implementation of the C++17 std::optional<> feature: http://en.cppreference.com/w/cpp/utility/optional BUG=521269 ==========
Dana, PTAL. I've applied your comments. I still need to add a few tests but I assumed you might want to have a look earlier than later. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode20 base/optional.h:20: explicit nullopt_t(int) { } On 2016/03/04 at 00:31:08, danakj wrote: > Can you throw a TODO on here that it should be constexptr once MSVC 2015? Done. > nit: no space in {}. did you git cl format? Done. I didn't use `git cl format`, will do. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode25 base/optional.h:25: const in_place_t in_place{}; On 2016/03/04 at 00:31:09, danakj wrote: > this is brace-initialization which we don't allow yet. you can just write this as below? > > const in_place_t in_place; > > Also, TODO for constexpr? Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode29 base/optional.h:29: const nullopt_t nullopt(0); On 2016/03/04 at 00:31:08, danakj wrote: > TODO for constexpr here too? Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode32 base/optional.h:32: // class: http://en.cppreference.com/w/cpp/experimental/optional/optional On 2016/03/04 at 00:31:08, danakj wrote: > Link should be http://en.cppreference.com/w/cpp/experimental/optional (you're linking to constructor) Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode38 base/optional.h:38: // - Private member |val| (for exposition) is not implemented. On 2016/03/04 at 00:31:08, danakj wrote: > The |val| is just an example, I don't think you need to mention it here. Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode39 base/optional.h:39: // - The destructor doesn't use std::std::is_trivially_destructible. On 2016/03/04 at 00:31:08, danakj wrote: > can you explain why? msvc problems? Didn't even work locally. I've added a TODO. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode40 base/optional.h:40: // - All the non-members are in the 'base' namespace instead of `std'.` On 2016/03/04 at 00:31:07, danakj wrote: > quotes got a little weird around "std" Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode45 base/optional.h:45: Optional(base::nullopt_t opt) : Optional() { } On 2016/03/04 at 00:31:08, danakj wrote: > nit: you could leave the parameter unnamed Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode54 base/optional.h:54: init(std::move(*other.buffer_.template data_as<T>())); On 2016/03/04 at 00:31:08, danakj wrote: > why not std::move(other.value()) ? it should cast the T& to a T&& to do what you want? Hmm, I think I've implemented this method before .value() returning T&&. Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode74 base/optional.h:74: Optional& operator=(base::nullopt_t opt) { On 2016/03/04 at 00:31:08, danakj wrote: > nit: can leave the parameter unnamed Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcode95 base/optional.h:95: init_or_assign(std::move(*other.buffer_.template data_as<T>())); On 2016/03/04 at 00:31:09, danakj wrote: > why not std::move(other.value()) ? it should cast the T& to a T&& to do what you want? As above. Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:102: <std::is_same<std::decay<U>, T>::value, Optional&>::type On 2016/03/04 at 00:31:07, danakj wrote: > The formatting here.. is that clang-format? No. This CL wasn't meant to be done but just for you to have a look. Will run `git cl format`. > Can you write this with the enable_if on the left side of the function name (instead of using auto -> syntax)? Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:110: return buffer_.template data_as<T>(); On 2016/03/04 at 00:31:07, danakj wrote: > return &value()? Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:115: return buffer_.template data_as<T>(); On 2016/03/04 at 00:31:07, danakj wrote: > return &value()? Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:127: return std::forward<T>(value()); On 2016/03/04 at 00:31:08, danakj wrote: > why forward? just std::move(value())? > > optional won't compile if T is a reference type for many reasons. Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:131: return std::forward<T>(value()); On 2016/03/04 at 00:31:09, danakj wrote: > std::move(value())? Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:148: typedef T* Optional::*Testable; On 2016/03/04 at 00:31:09, danakj wrote: > prefer "using" to "typedef" Done (deprecated). https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:153: return is_null_ ? nullptr : &Optional::buffer_.template data_as<T>(); On 2016/03/04 at 00:31:08, danakj wrote: > return is_null_ ? nullptr : &Optional::is_null_ ? > > > So.. I have good news and bad news. > > Bad news: Since this class uses ref qualifiers, it won't compile in MSVC 2013. So we'll have to wait for 2015. > Good news: 2015 continues to be very soon, I heard say this week again? Next week? idk > Better news: That means you can just use explicit operator, cuz that will work in MSVC 2015 too. \o/ I removed the dirty workaround. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:178: T value_or(U&& default_value) const { On 2016/03/04 at 00:31:08, danakj wrote: > can you add some static asserts here: > > // TODO(danakj): Enable this when MSVC 2015 happens. > // static_assert(std::is_copy_constructible<T>::value, "T must be copy constructible"); > static_assert(std::is_convertible<U, T>::value, "U must be convertible to T"); > > i'm assuming the 2nd one would work now.. Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:179: return is_null_ ? default_value : value(); On 2016/03/04 at 00:31:07, danakj wrote: > i think in the null case you need to actually return > > static_cast<T>(std::forward<U>(default_value)) Oups. I completely missed that in the spec. Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:183: T value_or(U&& default_value) { On 2016/03/04 at 00:31:08, danakj wrote: > can you add some static asserts here: > > // TODO(danakj): Enable this when MSVC 2015 happens. > // static_assert(std::is_move_constructible<T>::value, "T must be move constructible"); > static_assert(std::is_convertible<U, T>::value, "U must be convertible to T"); > > i'm assuming the 2nd one would work now.. Done. > this should be qualified as && Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:184: return is_null_ ? default_value : value(); On 2016/03/04 at 00:31:08, danakj wrote: > in null case: return static_cast<T>(std::forward<U>(default_value)) > in non-null case: return std::move(value()) Thanks for noticing. I missed this from the spec. Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:204: swap(**this, *other); On 2016/03/04 at 00:31:07, danakj wrote: > I think this has to be a typo in cppreference? It should be swap(value(), other.value())? The implementation in libcxx seems to agree. > > Is there a unit test case for this? Yes: OptionalTest.Swap_* https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:210: init(T(std::forward<Args>(args)...)); On 2016/03/04 at 00:31:09, danakj wrote: > This isn't quite the same thing as constructing from the arguments, this is constructing and then moving. You want to forward the args to init() (you'll need another init for this or skip using init() here) Done... I think :) https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:214: void init(const T& value) { On 2016/03/04 at 00:31:08, danakj wrote: > can you name these private methods in CamelCase style? They aren't necessarily that cheap. Done. However, my intention was to keep a consistent style with-in the file. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:222: new (buffer_.template data_as<T>()) T(std::forward<T>(value)); On 2016/03/04 at 00:31:08, danakj wrote: > just move() here, not forward. the function is not separately templated, so it will always be an rvalue ref. Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:235: init(std::forward<T>(value)); On 2016/03/04 at 00:31:09, danakj wrote: > just std::move() here Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:237: *buffer_.template data_as<T>() = std::forward<T>(value); On 2016/03/04 at 00:31:07, danakj wrote: > and here Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:253: if (bool(lhs) != bool(rhs)) On 2016/03/04 at 00:31:08, danakj wrote: > These are cast-styles that the google style guide doesn't like. Instead you can write static_cast<bool>(lhs) or !!lhs or lhs != nullopt. > > using nullopt is the most verbose but may also be the most clear, especially in cases where you are negating the bool cast. Done. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:329: return bool(opt); On 2016/03/04 at 00:31:08, danakj wrote: > can you write the > operators here and below in terms of the < operators above? I could but my goal here was to follow the spec as much as possible and the returned value as defined more or less as I wrote them here. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:394: return bool(opt) ? *opt < value : true; On 2016/03/04 at 00:31:08, danakj wrote: > same here, write these in terms of the operator < and friends? As above. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:417: // TODO: implement hash function. On 2016/03/04 at 00:31:08, danakj wrote: > put a name or a bug on it Implemented it.
Looks great! I think we'll probably be able to use constexpr this week, 2015 continues to stick so far. https://codereview.chromium.org/1245163002/diff/100001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/100001/base/optional.h#newcod... base/optional.h:329: return bool(opt); On 2016/03/18 01:16:03, Mounir Lamouri wrote: > On 2016/03/04 at 00:31:08, danakj wrote: > > can you write the > operators here and below in terms of the < operators > above? > > I could but my goal here was to follow the spec as much as possible and the > returned value as defined more or less as I wrote them here. I'd still prefer that you wrote them in terms of previously written operators. They should still match the spec behaviour. https://codereview.chromium.org/1245163002/diff/120001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcode68 base/optional.h:68: // TODO(mlamouri): use is_trivially_destructible when possible. Can you show what the error is when you use it? https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcod... base/optional.h:160: // TODO(mlamouri): switch to is_copy_convertible when possible. It should assert both when possible, not switch to. So just mention what it should be also asserting when is_copy_constructible exists. https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcod... base/optional.h:168: // TODO(mlamouri): switch to is_move_convertible when possible. ditto https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcod... base/optional.h:258: return rhs == nullopt ? false : lhs == nullopt ? true : *lhs < *rhs; can you use () around the nested expression? https://codereview.chromium.org/1245163002/diff/120001/base/optional_unittest.cc File base/optional_unittest.cc (right): https://codereview.chromium.org/1245163002/diff/120001/base/optional_unittest... base/optional_unittest.cc:823: std::hash<int> intHash; int_hash https://codereview.chromium.org/1245163002/diff/120001/base/optional_unittest... base/optional_unittest.cc:833: EXPECT_EQ(strHash(std::string("")), maybe some other string than empty? https://codereview.chromium.org/1245163002/diff/120001/base/optional_unittest... base/optional_unittest.cc:839: std::hash<Optional<int>> optIntHash; opt_int_hash
horo@chromium.org changed reviewers: + horo@chromium.org
https://codereview.chromium.org/1245163002/diff/120001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcode27 base/optional.h:27: constexpr in_place_t in_place; I think you need an explicit initializer to initialize 'in_place'. constexpr in_place_t in_place {};
All comments applied. I've also finished the unit tests. PTAL. https://codereview.chromium.org/1245163002/diff/120001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcode27 base/optional.h:27: constexpr in_place_t in_place; On 2016/03/24 at 06:25:58, horo wrote: > I think you need an explicit initializer to initialize 'in_place'. > > constexpr in_place_t in_place {}; danajk@ was against using this. https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcode68 base/optional.h:68: // TODO(mlamouri): use is_trivially_destructible when possible. On 2016/03/21 at 21:49:38, danakj wrote: > Can you show what the error is when you use it? Sure: In file included from ../../base/optional_unittest.cc:5: ../../base/optional.h:71:14: error: no template named 'is_trivially_destructible' in namespace 'std'; did you mean 'has_trivial_destructor'? if (std::is_trivially_destructible<T>::value) ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ has_trivial_destructor ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/type_traits:731:12: note: 'has_trivial_destructor' declared here struct has_trivial_destructor ^ https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcod... base/optional.h:160: // TODO(mlamouri): switch to is_copy_convertible when possible. On 2016/03/21 at 21:49:37, danakj wrote: > It should assert both when possible, not switch to. So just mention what it should be also asserting when is_copy_constructible exists. Done. https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcod... base/optional.h:168: // TODO(mlamouri): switch to is_move_convertible when possible. On 2016/03/21 at 21:49:38, danakj wrote: > ditto Done. https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcod... base/optional.h:258: return rhs == nullopt ? false : lhs == nullopt ? true : *lhs < *rhs; On 2016/03/21 at 21:49:37, danakj wrote: > can you use () around the nested expression? If I understood correctly, done. https://codereview.chromium.org/1245163002/diff/120001/base/optional_unittest.cc File base/optional_unittest.cc (right): https://codereview.chromium.org/1245163002/diff/120001/base/optional_unittest... base/optional_unittest.cc:823: std::hash<int> intHash; On 2016/03/21 at 21:49:38, danakj wrote: > int_hash Done. https://codereview.chromium.org/1245163002/diff/120001/base/optional_unittest... base/optional_unittest.cc:833: EXPECT_EQ(strHash(std::string("")), On 2016/03/21 at 21:49:38, danakj wrote: > maybe some other string than empty? Done. https://codereview.chromium.org/1245163002/diff/120001/base/optional_unittest... base/optional_unittest.cc:839: std::hash<Optional<int>> optIntHash; On 2016/03/21 at 21:49:38, danakj wrote: > opt_int_hash Done.
https://codereview.chromium.org/1245163002/diff/120001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcode27 base/optional.h:27: constexpr in_place_t in_place; On 2016/03/24 09:20:44, Mounir Lamouri wrote: > On 2016/03/24 at 06:25:58, horo wrote: > > I think you need an explicit initializer to initialize 'in_place'. > > > > constexpr in_place_t in_place {}; > > danajk@ was against using this. I'm not familiar with constexpr. But when I added '#include "base/optional.h' in ipc/ipc_message_utils.h for https://crrev.com/1731153005 I saw this error. danajk@ Could you please let me know how to avoid this error? --------------------- In file included from ../ipc/ipc_message_utils.h:25: ../base/optional.h:27:22: error: default initialization of an object of const type 'const base::in_place_t' without a user-provided default constructor constexpr in_place_t in_place; ^ ../base/optional.h:27:30: note: add an explicit initializer to initialize 'in_place' constexpr in_place_t in_place; ^ {} 1 error generated. ---------------------
On 2016/03/24 09:47:22, horo wrote: > https://codereview.chromium.org/1245163002/diff/120001/base/optional.h > File base/optional.h (right): > > https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcode27 > base/optional.h:27: constexpr in_place_t in_place; > On 2016/03/24 09:20:44, Mounir Lamouri wrote: > > On 2016/03/24 at 06:25:58, horo wrote: > > > I think you need an explicit initializer to initialize 'in_place'. > > > > > > constexpr in_place_t in_place {}; > > > > danajk@ was against using this. > > I'm not familiar with constexpr. > But when I added '#include "base/optional.h' in ipc/ipc_message_utils.h for > https://crrev.com/1731153005 I saw this error. > > danajk@ > Could you please let me know how to avoid this error? > > --------------------- > In file included from ../ipc/ipc_message_utils.h:25: > ../base/optional.h:27:22: error: default initialization of an object of const > type 'const base::in_place_t' without a user-provided default constructor > constexpr in_place_t in_place; > ^ > ../base/optional.h:27:30: note: add an explicit initializer to initialize > 'in_place' > constexpr in_place_t in_place; > ^ > {} > 1 error generated. > --------------------- This error happens while compiling for NACL binaries.
On 2016/03/24 at 09:56:30, horo wrote: > On 2016/03/24 09:47:22, horo wrote: > > https://codereview.chromium.org/1245163002/diff/120001/base/optional.h > > File base/optional.h (right): > > > > https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcode27 > > base/optional.h:27: constexpr in_place_t in_place; > > On 2016/03/24 09:20:44, Mounir Lamouri wrote: > > > On 2016/03/24 at 06:25:58, horo wrote: > > > > I think you need an explicit initializer to initialize 'in_place'. > > > > > > > > constexpr in_place_t in_place {}; > > > > > > danajk@ was against using this. > > > > I'm not familiar with constexpr. > > But when I added '#include "base/optional.h' in ipc/ipc_message_utils.h for > > https://crrev.com/1731153005 I saw this error. > > > > danajk@ > > Could you please let me know how to avoid this error? > > > > --------------------- > > In file included from ../ipc/ipc_message_utils.h:25: > > ../base/optional.h:27:22: error: default initialization of an object of const > > type 'const base::in_place_t' without a user-provided default constructor > > constexpr in_place_t in_place; > > ^ > > ../base/optional.h:27:30: note: add an explicit initializer to initialize > > 'in_place' > > constexpr in_place_t in_place; > > ^ > > {} > > 1 error generated. > > --------------------- > > This error happens while compiling for NACL binaries. I will have a look at this. I do not compile NACL on my laptop and I've been working on this on my laptop exclusively so it's very likely that I missed it.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1245163002/diff/120001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcode27 base/optional.h:27: constexpr in_place_t in_place; On 2016/03/24 at 09:47:22, horo wrote: > On 2016/03/24 09:20:44, Mounir Lamouri wrote: > > On 2016/03/24 at 06:25:58, horo wrote: > > > I think you need an explicit initializer to initialize 'in_place'. > > > > > > constexpr in_place_t in_place {}; > > > > danajk@ was against using this. > > I'm not familiar with constexpr. > But when I added '#include "base/optional.h' in ipc/ipc_message_utils.h for https://crrev.com/1731153005 I saw this error. > > danajk@ > Could you please let me know how to avoid this error? > > --------------------- > In file included from ../ipc/ipc_message_utils.h:25: > ../base/optional.h:27:22: error: default initialization of an object of const type 'const base::in_place_t' without a user-provided default constructor > constexpr in_place_t in_place; > ^ > ../base/optional.h:27:30: note: add an explicit initializer to initialize 'in_place' > constexpr in_place_t in_place; > ^ > {} > 1 error generated. > --------------------- Some alternatives: constexpr in_place_t in_place = in_place_t(); constexpr in_place_t in_place = {}; FWIW, I'd probably go with the latter: using {} like that to initialize PODs is a common and accepted idiom. I believe danakj@'s comment was specifically about the braced initialization syntax introduced in C++11 (which allows the '=' to be omitted): http://en.cppreference.com/w/cpp/language/aggregate_initialization
https://codereview.chromium.org/1245163002/diff/120001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcode27 base/optional.h:27: constexpr in_place_t in_place; On 2016/03/24 10:20:51, dcheng wrote: > On 2016/03/24 at 09:47:22, horo wrote: > > On 2016/03/24 09:20:44, Mounir Lamouri wrote: > > > On 2016/03/24 at 06:25:58, horo wrote: > > > > I think you need an explicit initializer to initialize 'in_place'. > > > > > > > > constexpr in_place_t in_place {}; > > > > > > danajk@ was against using this. > > > > I'm not familiar with constexpr. > > But when I added '#include "base/optional.h' in ipc/ipc_message_utils.h for > https://crrev.com/1731153005 I saw this error. > > > > danajk@ > > Could you please let me know how to avoid this error? > > > > --------------------- > > In file included from ../ipc/ipc_message_utils.h:25: > > ../base/optional.h:27:22: error: default initialization of an object of const > type 'const base::in_place_t' without a user-provided default constructor > > constexpr in_place_t in_place; > > ^ > > ../base/optional.h:27:30: note: add an explicit initializer to initialize > 'in_place' > > constexpr in_place_t in_place; > > ^ > > {} > > 1 error generated. > > --------------------- > > Some alternatives: > constexpr in_place_t in_place = in_place_t(); > constexpr in_place_t in_place = {}; > > FWIW, I'd probably go with the latter: using {} like that to initialize PODs is > a common and accepted idiom. I believe danakj@'s comment was specifically about > the braced initialization syntax introduced in C++11 (which allows the '=' to be > omitted): http://en.cppreference.com/w/cpp/language/aggregate_initialization = {} sounds good. https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcode68 base/optional.h:68: // TODO(mlamouri): use is_trivially_destructible when possible. On 2016/03/24 09:20:44, Mounir Lamouri wrote: > On 2016/03/21 at 21:49:38, danakj wrote: > > Can you show what the error is when you use it? > > Sure: > In file included from ../../base/optional_unittest.cc:5: > ../../base/optional.h:71:14: error: no template named > 'is_trivially_destructible' in namespace 'std'; did you mean > 'has_trivial_destructor'? > if (std::is_trivially_destructible<T>::value) > ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ > has_trivial_destructor > ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/type_traits:731:12: > note: 'has_trivial_destructor' declared here > struct has_trivial_destructor > ^ Oh, gcc is missing that, ok. We should add it to base/template_util.h then at some point to use it.
I am working on a fix that uses this patch ( see https://codereview.chromium.org/1837233002). The windows build is giving a compile time error as given below. Did you come across this error as well? e:\b\build\slave\win\build\src\testing\gtest\include\gtest\internal\gtest-internal.h(892): error C2718: 'base::Optional<base::`anonymous-namespace'::TestObject>': actual parameter with requested alignment of 8 won't be aligned e:\b\build\slave\win\build\src\base\optional_unittest.cc(104): note: see reference to class template instantiation 'testing::internal::ImplicitlyConvertible<base::Optional<base::`anonymous-namespace'::TestObject>,testing::AssertionResult>' being compiled e:\b\build\slave\win\build\src\base\optional_unittest.cc(106): error C2718: 'base::Optional<base::`anonymous-namespace'::TestObject>': actual parameter with requested alignment of 8 won't be aligned e:\b\build\slave\win\build\src\base\optional_unittest.cc(280): error C2718: 'base::Optional<base::`anonymous-namespace'::TestObject>': actual parameter with requested alignment of 8 won't be aligned e:\b\build\slave\win\build\src\base\optional_unittest.cc(341): error C2718: 'base::Optional<base::`anonymous-namespace'::TestObject>': actual parameter with requested alignment of 8 won't be aligned ninja: build stopped: subcommand failed. Thanks, Shivani
Description was changed from ========== Base: add Optional<T>. This is an implementation of the C++17 std::optional<> feature: http://en.cppreference.com/w/cpp/utility/optional BUG=521269 ========== to ========== Base: add Optional<T>. This is an implementation of the C++17 std::optional<> feature: http://en.cppreference.com/w/cpp/utility/optional BUG=521269 ==========
wfh@chromium.org changed reviewers: + wfh@chromium.org
hello I wondered what the status of this CL was as it's blocking another CL that is blocking the move of bots to GCE (amazingly). Now we are VS2015 is there anything blocking landing this?
On Tue, Apr 5, 2016 at 11:22 AM, <wfh@chromium.org> wrote: > hello I wondered what the status of this CL was as it's blocking another > CL that > is blocking the move of bots to GCE (amazingly). Now we are VS2015 is there > anything blocking landing this? > There has been some discussion about relying on 2015 features on cxx@, but we're not doing so just yet. It has not yet become clear that 2015 will stick for our entire release process. We're getting close though. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
wfh@chromium.org changed reviewers: + shivanisha@chromium.org
okay thanks for the update. +shivanisha
M51 has branched, I think this is a good indication that we are sticking with VS2015? I wondered if you had any updated ETA on when this CL could land?
On Mon, Apr 11, 2016 at 10:52 AM, <wfh@chromium.org> wrote: > M51 has branched, I think this is a good indication that we are sticking > with > VS2015? I wondered if you had any updated ETA on when this CL could land? > We haven't heard from PMO people yet. I suggest following the cxx@chromium.org threads that have been talking about this recently. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Looks like we're ok to use VS2015 now. This LGTM with 1 last comment. Were there any more tests you wanted to add? There have been a lot of people looking to use something like this, and we now have at least 3 "NullableFoo" things in our code base (and a 4th waiting for this) that should all be this. I'd like to send some kind of email to chromium-dev when this lands to tell people exist and give people an idea how to use it (ie, don't pass or return it from methods almost always, use it instead of scoped_ptr without the mallocs). https://codereview.chromium.org/1245163002/diff/120001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/120001/base/optional.h#newcode27 base/optional.h:27: constexpr in_place_t in_place; On 2016/03/24 20:38:51, danakj wrote: > On 2016/03/24 10:20:51, dcheng wrote: > > On 2016/03/24 at 09:47:22, horo wrote: > > > On 2016/03/24 09:20:44, Mounir Lamouri wrote: > > > > On 2016/03/24 at 06:25:58, horo wrote: > > > > > I think you need an explicit initializer to initialize 'in_place'. > > > > > > > > > > constexpr in_place_t in_place {}; > > > > > > > > danajk@ was against using this. > > > > > > I'm not familiar with constexpr. > > > But when I added '#include "base/optional.h' in ipc/ipc_message_utils.h for > > https://crrev.com/1731153005 I saw this error. > > > > > > danajk@ > > > Could you please let me know how to avoid this error? > > > > > > --------------------- > > > In file included from ../ipc/ipc_message_utils.h:25: > > > ../base/optional.h:27:22: error: default initialization of an object of > const > > type 'const base::in_place_t' without a user-provided default constructor > > > constexpr in_place_t in_place; > > > ^ > > > ../base/optional.h:27:30: note: add an explicit initializer to initialize > > 'in_place' > > > constexpr in_place_t in_place; > > > ^ > > > {} > > > 1 error generated. > > > --------------------- > > > > Some alternatives: > > constexpr in_place_t in_place = in_place_t(); > > constexpr in_place_t in_place = {}; > > > > FWIW, I'd probably go with the latter: using {} like that to initialize PODs > is > > a common and accepted idiom. I believe danakj@'s comment was specifically > about > > the braced initialization syntax introduced in C++11 (which allows the '=' to > be > > omitted): http://en.cppreference.com/w/cpp/language/aggregate_initialization > > = {} sounds good. Let's do this ^ still.
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245163002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245163002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
When you EXPECT_EQ(a,b) or EXPECT_TRUE(a) and |a| and |b| are of type Optional<TestObject> then we get the error on windows: e:\b\build\slave\win\build\src\base\optional_unittest.cc(106): error C2718: 'base::Optional<base::`anonymous-namespace'::TestObject>': actual parameter with requested alignment of 8 won't be aligned I'm not actually sure if this is cuz of the gtest machinery or not. What happens if you do EXPECT_TRUE(!!a) or EXPECT_TRUE(a == b)?
On 2016/04/13 at 00:30:33, danakj wrote: > When you EXPECT_EQ(a,b) or EXPECT_TRUE(a) and |a| and |b| are of type Optional<TestObject> then we get the error on windows: > > e:\b\build\slave\win\build\src\base\optional_unittest.cc(106): error C2718: 'base::Optional<base::`anonymous-namespace'::TestObject>': actual parameter with requested alignment of 8 won't be aligned > > I'm not actually sure if this is cuz of the gtest machinery or not. What happens if you do EXPECT_TRUE(!!a) or EXPECT_TRUE(a == b)? https://msdn.microsoft.com/en-us/library/sxe76d9e.aspx suggests Microsoft just doesn't allow anything containing aligned memory to be a parameter? MSVC 2015 also supports unrestricted unions, so it would be easy to see if cl.exe objects to that implementation, if all else fails.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245163002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245163002/180001
Dana, I've updated the CL with some doc. It's still WIP but I have a couple of meetings coming so hopefully you can have a quick look and I can iterate on it after. If you could leave some insights with regards to technical reasons why base::Optional<T> shouldn't be used in some cases, it would help :) (Actually, I thought that returning an Optional<T> from a function should be fine but you said otherwise.)
Thanks looks good, and looks like the windows bot is happy too. https://codereview.chromium.org/1245163002/diff/240001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/240001/base/optional.h#newcode34 base/optional.h:34: // http://en.cppreference.com/w/cpp/utility/optional Point to the markdown as well? https://codereview.chromium.org/1245163002/diff/240001/docs/optional.md File docs/optional.md (right): https://codereview.chromium.org/1245163002/diff/240001/docs/optional.md#newco... docs/optional.md:88: TODO: list cases and explain why. Talked a bit with jyasskin about this, I think the one thing we should point out is: "Don't use it as an argument to a function, as it forces the caller to use an Optional<T>. Instead continue to do something like T* for arguments that can be omitted."
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245163002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245163002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Documentation updated. https://codereview.chromium.org/1245163002/diff/240001/base/optional.h File base/optional.h (right): https://codereview.chromium.org/1245163002/diff/240001/base/optional.h#newcode34 base/optional.h:34: // http://en.cppreference.com/w/cpp/utility/optional On 2016/04/14 at 20:16:20, danakj wrote: > Point to the markdown as well? Done. https://codereview.chromium.org/1245163002/diff/240001/docs/optional.md File docs/optional.md (right): https://codereview.chromium.org/1245163002/diff/240001/docs/optional.md#newco... docs/optional.md:88: TODO: list cases and explain why. On 2016/04/14 at 20:16:20, danakj wrote: > Talked a bit with jyasskin about this, I think the one thing we should point out is: "Don't use it as an argument to a function, as it forces the caller to use an Optional<T>. Instead continue to do something like T* for arguments that can be omitted." Done. Also mentioned the MSVC issue.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245163002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245163002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM let's land and start converting existing Nullable/Optional/Maybe things to use this. https://codereview.chromium.org/1245163002/diff/260001/docs/optional.md File docs/optional.md (right): https://codereview.chromium.org/1245163002/diff/260001/docs/optional.md#newco... docs/optional.md:91: It is recommended to not use `base::Optional<T>` as a function parameter as it will force the FWIW, I'm not sure what line length you used but 80 col seems more optimal.
Description was changed from ========== Base: add Optional<T>. This is an implementation of the C++17 std::optional<> feature: http://en.cppreference.com/w/cpp/utility/optional BUG=521269 ========== to ========== Base: add Optional<T>. This is an implementation of the C++17 std::optional<> feature: http://en.cppreference.com/w/cpp/utility/optional Chromium documentation: https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md BUG=521269 ==========
The CQ bit was checked by mlamouri@chromium.org
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/1245163002/#ps280001 (title: "doc changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245163002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245163002/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Base: add Optional<T>. This is an implementation of the C++17 std::optional<> feature: http://en.cppreference.com/w/cpp/utility/optional Chromium documentation: https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md BUG=521269 ========== to ========== Base: add Optional<T>. This is an implementation of the C++17 std::optional<> feature: http://en.cppreference.com/w/cpp/utility/optional Chromium documentation: https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md BUG=521269 Committed: https://crrev.com/8756e4f20df53bbb7fbb49b2ca19fda93a95bdb4 Cr-Commit-Position: refs/heads/master@{#388187} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/8756e4f20df53bbb7fbb49b2ca19fda93a95bdb4 Cr-Commit-Position: refs/heads/master@{#388187}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/1902833002/ by cmumford@chromium.org. The reason for reverting is: Unit tests failing on Windows-7-SP1. https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....
The CQ bit was checked by mlamouri@chromium.org
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/1245163002/#ps320001 (title: "fix md")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245163002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245163002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Base: add Optional<T>. This is an implementation of the C++17 std::optional<> feature: http://en.cppreference.com/w/cpp/utility/optional Chromium documentation: https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md BUG=521269 Committed: https://crrev.com/8756e4f20df53bbb7fbb49b2ca19fda93a95bdb4 Cr-Commit-Position: refs/heads/master@{#388187} ========== to ========== Base: add Optional<T>. This is an implementation of the C++17 std::optional<> feature: http://en.cppreference.com/w/cpp/utility/optional Chromium documentation: https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md BUG=521269 Committed: https://crrev.com/8756e4f20df53bbb7fbb49b2ca19fda93a95bdb4 Cr-Commit-Position: refs/heads/master@{#388187} Committed: https://crrev.com/53f6b258355ce36752ea7a8dcf8953560b5be818 Cr-Commit-Position: refs/heads/master@{#388232} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/53f6b258355ce36752ea7a8dcf8953560b5be818 Cr-Commit-Position: refs/heads/master@{#388232} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
