[libc++] Implement D2351R0 "Mark all library static cast wrappers as [[nodiscard]]"

[• Quuxplusone]

Summary

These [[nodiscard]] annotations are added as a conforming extension;
it's unclear whether the paper will actually be adopted and make them
mandatory, but they do seem like good ideas regardless.

https://isocpp.org/files/papers/D2351R0.pdf

This patch implements the paper's effect on:
- std::to_integer, std::to_underlying
- std::forward, std::move, std::move_if_noexcept
- std::as_const
- std::identity

The paper also affects (but libc++ does not yet have an implementation of):
- std::bit_cast

[curdeius]

LGTM. It can indeed be seen as a QoI change.

[ldionne]

Can we add libc++ specific tests for those? A simple .verify.cpp test for each should be fairly easy to add.

[• Quuxplusone]

@ldionne: Good point. It turns out that there's a dedicated place to hang these new test cases! (Easily found by grepping libcxx/test/ for "nodiscard", once you'd pointed it out.)

I decided to go ahead and mention std::bit_cast in the test file, under a code comment, so that maybe whoever adds bit_cast will be grepping for it and find these tests and update them.

[ldionne]

In D99895#2669617, @Quuxplusone wrote:

@ldionne: Good point. It turns out that there's a dedicated place to hang these new test cases! (Easily found by grepping libcxx/test/ for "nodiscard", once you'd pointed it out.)

I decided to go ahead and mention std::bit_cast in the test file, under a code comment, so that maybe whoever adds bit_cast will be grepping for it and find these tests and update them.

Re: bit_cast, my implementation was stuck on compiler bugs last time I checked. See D75960.

[• Quuxplusone]

Fix forward.fail.cpp, which checks the wording of the static_assert "can not" typo that I fixed incidentally.
Poke buildkite.

libcxx/docs/UsingLibcxx.rst

 
 For this reason libc++ provides an extension that does just that! The
 extension must be enabled by defining ``_LIBCPP_ENABLE_NODISCARD``. The extended
 applications of ``[[nodiscard]]`` takes two forms:
 
 1. Backporting ``[[nodiscard]]`` to entities declared as such by the
    standard in newer dialects, but not in the present one.
 
-2. Extended applications of ``[[nodiscard]]``, at the libraries discretion,
+2. Extended applications of ``[[nodiscard]]``, at the library's discretion,
    applied to entities never declared as such by the standard.
 
 Users may also opt-out of additional applications ``[[nodiscard]]`` using
 additional macros.
 
 Applications of the first form, which backport ``[[nodiscard]]`` from a newer
-dialect may be disabled using macros specific to the dialect it was added. For
-example ``_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17``.
+dialect, may be disabled using macros specific to the dialect in which it was
+added. For example, ``_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17``.
 
 Applications of the second form, which are pure extensions, may be disabled
 by defining ``_LIBCPP_DISABLE_NODISCARD_EXT``.
 
 
 Entities declared with ``_LIBCPP_NODISCARD_EXT``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

 * ``none_of``
 * ``remove_if``
 * ``remove``
 * ``search_n``
 * ``search``
 * ``unique``
 * ``upper_bound``
 * ``lock_guard``'s constructors
+* ``as_const``
+* ``forward``
+* ``move``
+* ``move_if_noexcept``
+* ``identity::operator()``
+* ``to_integer``
+* ``to_underlying``

libcxx/include/cstddef

template <class _Integer>
   { return __lhs = __lhs >> __shift; }
 
 template <class _Integer>
   constexpr _EnableByteOverload<_Integer>
   operator>> (byte  __lhs, _Integer __shift) noexcept
   { return static_cast<byte>(static_cast<unsigned char>(static_cast<unsigned int>(__lhs) >> __shift)); }
 
 template <class _Integer, class = _EnableByteOverload<_Integer> >
-  constexpr _Integer
+  _LIBCPP_NODISCARD_EXT constexpr _Integer
   to_integer(byte __b) noexcept { return static_cast<_Integer>(__b); }
 }
 
 #endif
 
 #endif  // _LIBCPP_CSTDDEF

libcxx/include/functional

 template <class _Tp>
 using unwrap_ref_decay_t = typename unwrap_ref_decay<_Tp>::type;
 #endif // > C++17
 
 #if _LIBCPP_STD_VER > 17
 // [func.identity]
 struct identity {
     template<class _Tp>
-    constexpr _Tp&& operator()(_Tp&& __t) const noexcept
+    _LIBCPP_NODISCARD_EXT constexpr _Tp&& operator()(_Tp&& __t) const noexcept
     {
         return _VSTD::forward<_Tp>(__t);
     }
 
     using is_transparent = void;
 };
 #endif // _LIBCPP_STD_VER > 17
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif  // _LIBCPP_FUNCTIONAL

libcxx/include/type_traits

     = is_destructible<_Tp>::value;
 #endif
 
 #endif // __has_keyword(__is_destructible)
 
 // move
 
 template <class _Tp>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+_LIBCPP_NODISCARD_EXT inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
 typename remove_reference<_Tp>::type&&
 move(_Tp&& __t) _NOEXCEPT
 {
     typedef _LIBCPP_NODEBUG_TYPE typename remove_reference<_Tp>::type _Up;
     return static_cast<_Up&&>(__t);
 }
 
 template <class _Tp>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+_LIBCPP_NODISCARD_EXT inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
 _Tp&&
 forward(typename remove_reference<_Tp>::type& __t) _NOEXCEPT
 {
     return static_cast<_Tp&&>(__t);
 }
 
 template <class _Tp>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+_LIBCPP_NODISCARD_EXT inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
 _Tp&&
 forward(typename remove_reference<_Tp>::type&& __t) _NOEXCEPT
 {
     static_assert(!is_lvalue_reference<_Tp>::value,
-                  "can not forward an rvalue as an lvalue");
+                  "cannot forward an rvalue as an lvalue");
     return static_cast<_Tp&&>(__t);
 }
 
 template <class _Tp>
 inline _LIBCPP_INLINE_VISIBILITY
 typename decay<_Tp>::type
 __decay_copy(_Tp&& __t)
 {

libcxx/include/utility

 
 // swap_ranges is defined in <type_traits>`
 
 // swap is defined in <type_traits>
 
 // move_if_noexcept
 
 template <class _Tp>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
+_LIBCPP_NODISCARD_EXT inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
 #ifndef _LIBCPP_CXX03_LANG
 typename conditional
 <
     !is_nothrow_move_constructible<_Tp>::value && is_copy_constructible<_Tp>::value,
     const _Tp&,
     _Tp&&
 >::type
 #else  // _LIBCPP_CXX03_LANG
 const _Tp&
 #endif
 move_if_noexcept(_Tp& __x) _NOEXCEPT
 {
     return _VSTD::move(__x);
 }
 
 #if _LIBCPP_STD_VER > 14
-template <class _Tp> constexpr add_const_t<_Tp>& as_const(_Tp& __t) noexcept { return __t; }
-template <class _Tp>                        void as_const(const _Tp&&) = delete;
+template <class _Tp>
+_LIBCPP_NODISCARD_EXT constexpr add_const_t<_Tp>& as_const(_Tp& __t) noexcept { return __t; }
+
+template <class _Tp>
+void as_const(const _Tp&&) = delete;
 #endif
 
 struct _LIBCPP_TEMPLATE_VIS piecewise_construct_t { explicit piecewise_construct_t() = default; };
 #if defined(_LIBCPP_CXX03_LANG) || defined(_LIBCPP_BUILDING_LIBRARY)
 extern _LIBCPP_EXPORTED_FROM_ABI const piecewise_construct_t piecewise_construct;// = piecewise_construct_t();
 #else
 /* _LIBCPP_INLINE_VAR */ constexpr piecewise_construct_t piecewise_construct = piecewise_construct_t();
 #endif

 _LIBCPP_INLINE_VISIBILITY constexpr typename underlying_type<_Tp>::type
 __to_underlying(_Tp __val) noexcept {
   return static_cast<typename underlying_type<_Tp>::type>(__val);
 }
 #endif // !_LIBCPP_CXX03_LANG
 
 #if _LIBCPP_STD_VER > 20
 template <class _Tp>
-_LIBCPP_INLINE_VISIBILITY constexpr underlying_type_t<_Tp>
+_LIBCPP_NODISCARD_EXT _LIBCPP_INLINE_VISIBILITY constexpr underlying_type_t<_Tp>
 to_underlying(_Tp __val) noexcept {
   return _VSTD::__to_underlying(__val);
 }
 #endif
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif  // _LIBCPP_UTILITY

libcxx/test/libcxx/diagnostics/nodiscard_extensions.pass.cpp

 // UNSUPPORTED: apple-clang-9
 // UNSUPPORTED: gcc-5
 
 // All entities to which libc++ applies [[nodiscard]] as an extension should
 // be tested here and in nodiscard_extensions.fail.cpp. They should also
 // be listed in `UsingLibcxx.rst` in the documentation for the extension.
 
 #include <algorithm>
-#include <functional>
+#include <cstddef> // to_integer
+#include <functional> // identity
 #include <iterator>
 #include <memory>
+#include <utility> // to_underlying
 
 #include "test_macros.h"
 
 struct P {
   bool operator()(int) const { return false; }
 };
 
-int main(int, char**) {
+void test_algorithms() {
   int arr[1] = { 1 };
 
   std::adjacent_find(std::begin(arr), std::end(arr));
   std::adjacent_find(std::begin(arr), std::end(arr), std::greater<int>());
   std::all_of(std::begin(arr), std::end(arr), P());
   std::any_of(std::begin(arr), std::end(arr), P());
   std::binary_search(std::begin(arr), std::end(arr), 1);
   std::binary_search(std::begin(arr), std::end(arr), 1, std::greater<int>());

 #if TEST_STD_VER >= 17
   std::search(std::begin(arr), std::end(arr),
               std::default_searcher(std::begin(arr), std::end(arr)));
 #endif
   std::unique(std::begin(arr), std::end(arr));
   std::unique(std::begin(arr), std::end(arr), std::greater<int>());
   std::upper_bound(std::begin(arr), std::end(arr), 1);
   std::upper_bound(std::begin(arr), std::end(arr), 1, std::greater<int>());
+}
+
+template<class LV, class RV>
+void test_template_cast_wrappers(LV&& lv, RV&& rv) {
+  std::forward<LV>(lv);
+  std::forward<RV>(rv);
+  std::move(lv);
+  std::move(rv);
+  std::move_if_noexcept(lv);
+  std::move_if_noexcept(rv);
+
+#if TEST_STD_VER >= 17
+  std::as_const(lv);
+  std::as_const(rv);
+#endif
+
+#if TEST_STD_VER >= 20
+  std::identity()(lv);
+  std::identity()(rv);
+#endif
+}
+
+void test_nontemplate_cast_wrappers()
+{
+#if TEST_STD_VER >= 17
+  std::byte b{42};
+  std::to_integer<int>(b);
+#endif
+
+#if TEST_STD_VER >= 20
+  // std::bit_cast<unsigned int>(42);
+#endif
+
+#if TEST_STD_VER > 20
+  enum E { Apple, Orange } e = Apple;
+  std::to_underlying(e);
+#endif
+}
+
+int main(int, char**) {
+  test_algorithms();
+
+  int i = 42;
+  test_template_cast_wrappers(i, std::move(i));
+  test_nontemplate_cast_wrappers();
 
   return 0;
 }

libcxx/test/libcxx/diagnostics/nodiscard_extensions.verify.cpp

 
 // All entities to which libc++ applies [[nodiscard]] as an extension should
 // be tested here and in nodiscard_extensions.pass.cpp. They should also
 // be listed in `UsingLibcxx.rst` in the documentation for the extension.
 
 // ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_NODISCARD
 
 #include <algorithm>
-#include <functional>
+#include <cstddef> // to_integer
+#include <functional> // identity
 #include <iterator>
 #include <memory>
+#include <utility> // to_underlying
 
 #include "test_macros.h"
 
 struct P {
   bool operator()(int) const { return false; }
 };
 
-int main(int, char**) {
+void test_algorithms() {
   int arr[1] = { 1 };
 
   // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
   std::adjacent_find(std::begin(arr), std::end(arr));
 
   // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
   std::adjacent_find(std::begin(arr), std::end(arr), std::greater<int>());
 
#endif
   // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
   std::unique(std::begin(arr), std::end(arr), std::greater<int>());
 
   // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
   std::upper_bound(std::begin(arr), std::end(arr), 1);
 
   // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
   std::upper_bound(std::begin(arr), std::end(arr), 1, std::greater<int>());
+}
+
+template<class LV, class RV>
+void test_template_cast_wrappers(LV&& lv, RV&& rv) {
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::forward<LV>(lv);
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::forward<RV>(rv);
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::move(lv);
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::move(rv);
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::move_if_noexcept(lv);
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::move_if_noexcept(rv);
+
+#if TEST_STD_VER >= 17
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::as_const(lv);
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::as_const(rv);
+#endif
+
+#if TEST_STD_VER >= 20
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::identity()(lv);
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::identity()(rv);
+#endif
+}
+
+void test_nontemplate_cast_wrappers()
+{
+#if TEST_STD_VER >= 17
+  std::byte b{42};
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::to_integer<int>(b);
+#endif
+
+#if TEST_STD_VER >= 20
+  // std::bit_cast<unsigned int>(42);
+#endif
+
+#if TEST_STD_VER > 20
+  enum E { Apple, Orange } e = Apple;
+  // expected-warning-re@+1 {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
+  std::to_underlying(e);
+#endif
+}
+
+int main(int, char**) {
+  test_algorithms();
+
+  int i = 42;
+  test_template_cast_wrappers(i, std::move(i));
+  test_nontemplate_cast_wrappers();
 
   return 0;
 }

libcxx/test/std/utilities/utility/forward/forward.fail.cpp

 
 A source() {return A();}
 const A csource() {return A();}
 
 int main(int, char**)
 {
     {
         std::forward<A&>(source());  // expected-note {{requested here}}
-        // expected-error-re@type_traits:* 1 {{static_assert failed{{.*}} "can not forward an rvalue as an lvalue"}}
+        // expected-error-re@type_traits:* 1 {{static_assert failed{{.*}} "cannot forward an rvalue as an lvalue"}}
     }
     {
         const A ca = A();
         std::forward<A&>(ca); // expected-error {{no matching function for call to 'forward'}}
     }
     {
         std::forward<A&>(csource());  // expected-error {{no matching function for call to 'forward'}}
     }