[libcxx-commits] [PATCH] D102119: [libcxx][optional] adds missing constexpr operations

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

This mostly looks good to me. A few comments, but mainly minor things.

The following two tests look like they still need to be updated:

- `libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/explicit_optional_U.pass.cpp`
- `libcxx/test/std/utilities/optional/optional.object/optional.object.assign/optional_U.pass.cpp`



================
Comment at: libcxx/docs/Cxx2bStatusPaperStatus.csv:13
 "`P2259R1 <https://wg21.link/P2259R1>`__","LWG","Repairing input range adaptors and counted_iterator","February 2021","",""
+"`P2231R1 <https://wg21.link/P2231R1>`__","LWG","Missing constexpr in std::optional and std::variant","February 2021","|In progress|","13.0"
 "","","","","",""
----------------
Was this paper accepted yet?


================
Comment at: libcxx/include/optional:106
     // 23.6.3.3, assignment
     optional &operator=(nullopt_t) noexcept;
     optional &operator=(const optional &);                // constexpr in C++20
----------------
This should be updated, too.


================
Comment at: libcxx/include/optional:109
     optional &operator=(optional &&) noexcept(see below); // constexpr in C++20
     template <class U = T> optional &operator=(U &&);
     template <class U> optional &operator=(const optional<U> &);
----------------
And these assignment operators. 


================
Comment at: libcxx/include/optional:326
+#if _LIBCPP_STD_VER > 17
+        _VSTD::construct_at(_VSTD::addressof(this->__val_), _VSTD::forward<_Args>(__args)...);
+#else
----------------
Is this part of another paper? Should we add a test too?


================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.assign/emplace_initializer_list.pass.cpp:71
 
+TEST_CONSTEXPR_CXX20 bool check_X()
+{
----------------
This could just be `constexpr`, no?


================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.assign/emplace_initializer_list.pass.cpp:85
+
+TEST_CONSTEXPR_CXX20 bool check_Y()
+{
----------------
Nit: `checkY` or `testY` (better, imho). 


================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/move.fail.cpp:23
 //    Except in the case where it is moving from an empty optional - that could be
 //    made to be constexpr (and libstdc++ does so).
 
----------------
Want to update this comment?


================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.dtor/dtor.pass.cpp:43
         static_assert(std::is_trivially_destructible<optional<T>>::value, "");
-        static_assert(std::is_literal_type<optional<T>>::value, "");
     }
----------------
We should still test this pre-c++20, I think. You could group them all together at the end, though.


================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.swap/swap.pass.cpp:137
+    static_assert(check_swap<W>());
+#endif
+
----------------
I think the tests here are sufficient. But if you //wanted//, you could use the same dtor count trick as you did above (using a pointer) and then factor the existing tests out into `check_swap` and get rid of `W`. 

As I said, either way is OK. I think there's sufficient set coverage here. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102119/new/

https://reviews.llvm.org/D102119