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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/optional:72
   // 23.6.9, specialized algorithms
-  template <class T> void swap(optional<T>&, optional<T>&) noexcept(see below );
+  template <class T> void swap(optional<T>&, optional<T>&) noexcept(see below ); // constexpr in C++20
   template <class T> constexpr optional<see below > make_optional(T&&);
----------------
cjdb wrote:
> ldionne wrote:
> > Should be `// constexpr in C++2b (in C++20 as an extension)` or something along those lines, just to acknowledge that we're supporting that as an extension.
> Is it really an extension if LWG are recommending we backport it to C++20 as a "fix"?
You're right - if the Standard says to backport it, then it's not an extension.


================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.assign/emplace.pass.cpp:233
         test_one_arg<const T>();
+#if TEST_STD_VER >= 20 && !defined(TEST_COMPILER_GCC)
+        static_assert(test_one_arg<T>());
----------------
cjdb wrote:
> zoecarver wrote:
> > cjdb wrote:
> > > ldionne wrote:
> > > > This should be:
> > > > 
> > > > ```
> > > > #if TEST_STD_VER > 20 || (TEST_STD_VER >= 20 && defined(_LIBCPP_VER))
> > > > ```
> > > > 
> > > > Basically, test it in C++2b, or in C++20 but only when testing libc++, since it is out own extension. Otherwise, a conforming implementation will fail the test suite in C++20 mode because they don't implement `constexpr` for `std::optional`. This applies everywhere.
> > > > 
> > > > Also, what's the deal with GCC? If it fails those tests, we should first report the issue to GCC and then use some `XFAIL` markup to disable it instead.
> > > > Also, what's the deal with GCC? If it fails those tests, we should first report the issue to GCC and then use some `XFAIL` markup to disable it instead.
> > > 
> > > GCC 10 doesn't like our constexpr `construct_at` (GCC 11 is okay with it). I wasn't comfortable saying GCC couldn't test this file //at all// given it still works outside of constant expressions.
> > I think you should mark the whole file as `UNSUPPORTED: gcc-10`. IMO it's better to test everything on one version of GCC than some things on no versions of GCC. 
> > 
> > Do we still support GCC 10? When does that change go through? And when it does, can/should we remove all this `UNSUPPORTED` cruft?
> > Do we still support GCC 10?
> 
> We only support GCC 10.
> 
> > And when it does, can/should we remove all this `UNSUPPORTED` cruft?
> 
> Why? It's not hurting anything to keep it there.
I would prefer that we use `UNSUPPORTED: gcc-10`. The reason is that `UNSUPPORTED` annotations disable the test for the given version of GCC which we know doesn't support <some feature>, but it doesn't disable it forever. When we start testing on GCC 11, the test will not be considered as `UNSUPPORTED`. This is a great way to make sure that our tests don't become useless in a given configuration - it will automatically start breaking in the new version, and we'll have to re-acknowledge the failure by adjusting the `UNSUPPORTED` markup.

On the contrary, guarding with `TEST_COMPILER_GCC` means that we'll most likely forget about this in the future, and those bits will never be tested on GCC.


================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.assign/nullopt_t.pass.cpp:25
 
+#if TEST_STD_VER >= 20
+consteval bool test_consteval()
----------------
Why aren't we simply constexpr-ifying the tests like you did elsewhere?


================
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, "");
     }
----------------
tcanens wrote:
> cjdb wrote:
> > zoecarver wrote:
> > > We should still test this pre-c++20, I think. You could group them all together at the end, though.
> > 🤷 `std::is_literal_type` was deprecated in C++17 with the rationale
> > 
> > > The traits had unreliable or awkward interfaces. The `is_literal_type` trait provided no way to detect which subset of constructors and member functions of a type were declared constexpr.
> > 
> > I'm not sure our tests should have something unreliable?
> The problem with `is_literal_type` is that it provides an answer to a mostly useless question - knowing that a type has //some// constexpr constructor tells you pretty much nothing about what you can do with it. The trait itself is perfectly reliable.
I think Chris meant that our tests shouldn't be using a tool that we deprecated and removed. I agree with him, I don't think it's useful to check that `optional<T>` is a literal type (but my mind can easily be changed if you tell me why it would be useful to check this).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102119