[libc++] Enable [[nodiscard]] extensions by default

[philnik]

Summary

Adding [[nodiscard]] to functions is a conforming extension and done extensively in the MSVC STL.

[EricWF]

LGTM once we get this clean.

Thanks for providing a way to turn it off.

[jloser]

libcxx/docs/UsingLibcxx.rst

358–359

This needs to read "can be disabled by defining `_LIBCPP_DISABLE_NODISCARD`" (note disable, not enable in the macro identifier)

[ldionne]

If we want to move forward with this patch, we're missing a bunch of changes. Please grep for _LIBCPP_ENABLE_NODISCARD in the codebase.

Once you've re-uploaded, I'll run a build internally to give us an idea of how the impact it might have on users. I suggest we discuss this once we have more data in hand.

BTW, if we find out that it doesn't break much code, and if we all agree that it's useful to catch bugs, then perhaps it doesn't make sense to provide an escape hatch at all.

[philnik]

• Address comments

[ldionne]

libcxx/include/__config

883

This whole thing is really confusing, and I think we need to clean it up. We have:

_LIBCPP_DISABLE_NODISCARD
_LIBCPP_DISABLE_NODISCARD_EXT
_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17

I don't understand why we have all that. We should only have one knob: _LIBCPP_DISABLE_NODISCARD_EXT, which would disable the nodiscards that we added as an extension.

[philnik]

• Rebased

[ldionne]

libcxx/docs/UsingLibcxx.rst

357–358

Let's break the line here.

libcxx/include/__config

889

I think this is what we mean:

1. In C++20, always mark those things required by the standard as nodiscard
2. In < C++20, always mark as nodiscard unless the user has opted out by defining _LIBCPP_DISABLE_NODISCARD_EXT.

892

(not attached to any particular line)

I think we need to go through all of our tests for nodiscard. Many of them will currently be marked as // UNSUPPORTED: c++03, c++11, c++14. Instead, they should be changed to: // UNSUPPORTED: (c++03 || c++11 || c++14) && !stdlib=libc++ so that we gain coverage for this newly added behavior.

[philnik]

• Address comments

[philnik]

• Try to fix CI

[philnik]

• Next try

[ldionne]

@cjdb You finally had your way :-)

I think this is a good change, the only things this uncovered when I tested in an internal build were actual bugs.

[ldionne]

Pinging vendors for awareness, since this may start breaking builds that compile with -Werror.

libcxx/docs/ReleaseNotes.rst

- The ``_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION`` macro has been added to allow
   re-enabling the ``allocator<void>`` specialization. When used in conjuction with
   ``_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS``, this ensures that the members of
   ``allocator<void>`` removed in C++20 can be accessed.
 
 - The experimental versions of ``boyer_moore_searcher`` and ``boyer_moore_horspool_searcher``
   will be removed in LLVM 17. You can disable the deprecation warnings by defining
   ``_LIBCPP_NO_EXPERIMENTAL_DEPRECATION_WARNING_SEARCHERS``.
 
+- Extended applications of ``[[nodiscard]]`` are now enabled by default. They can be
+  disabled by defining ``_LIBCPP_DISBALE_NODISCARD``.
+
 ABI Changes
 -----------
 
 - The ``_LIBCPP_ABI_USE_CXX03_NULLPTR_EMULATION`` macro controlling whether we use an
   emulation for ``std::nullptr_t`` in C++03 mode has been removed. After this change,
   ``_LIBCPP_ABI_USE_CXX03_NULLPTR_EMULATION`` will not be honoured anymore and there
   will be no way to opt back into the C++03 emulation of ``std::nullptr_t``.
 

libcxx/docs/UsingLibcxx.rst

**_LIBCPP_NO_VCRUNTIME**:
   prevents libc++ from depending on vcruntime headers. Consequently, it also
   prevents libc++ headers from being interoperable with vcruntime headers (from
   the aforementioned clashes), so users of this macro are promising to not
   attempt to combine libc++ headers with the problematic vcruntime headers. This
   macro also currently prevents certain `operator new`/`operator delete`
   replacement scenarios from working, e.g. replacing `operator new` and
   expecting a non-replaced `operator new[]` to call the replaced `operator new`.
 
-**_LIBCPP_ENABLE_NODISCARD**:
-  Allow the library to add ``[[nodiscard]]`` attributes to entities not specified
-  as ``[[nodiscard]]`` by the current language dialect. This includes
-  backporting applications of ``[[nodiscard]]`` from newer dialects and
-  additional extended applications at the discretion of the library. All
-  additional applications of ``[[nodiscard]]`` are disabled by default.
+**_LIBCPP_DISABLE_NODISCARD**:
+  Add ``[[nodiscard]]`` attributes only to entities specified as ``[[nodiscard]]``
+  by the current language dialect. This disables backporting applications
+  of ``[[nodiscard]]`` from newer dialects and additional extended applications
+  at the discretion of the library.
   See :ref:`Extended Applications of [[nodiscard]] <nodiscard extension>` for
   more information.
 
 **_LIBCPP_DISABLE_NODISCARD_EXT**:
   This macro prevents the library from applying ``[[nodiscard]]`` to entities
   purely as an extension. See :ref:`Extended Applications of [[nodiscard]] <nodiscard extension>`
   for more information.
 

 
 The ``[[nodiscard]]`` attribute is intended to help users find bugs where
 function return values are ignored when they shouldn't be. After C++17 the
 C++ standard has started to declared such library functions as ``[[nodiscard]]``.
 However, this application is limited and applies only to dialects after C++17.
 Users who want help diagnosing misuses of STL functions may desire a more
 liberal application of ``[[nodiscard]]``.
 
 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:
+extension is enabled by default and can be disabled by defining ``_LIBCPP_DISABLE_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 library's discretion,
    applied to entities never declared as such by the standard.
 
 Users may also opt-out of additional applications ``[[nodiscard]]`` using

libcxx/include/__config

 // We can't use GCC's [[gnu::warn_unused_result]] and
 // __attribute__((warn_unused_result)), because GCC does not silence them via
 // (void) cast.
 #    define _LIBCPP_NODISCARD
 #  endif
 
 // _LIBCPP_NODISCARD_EXT may be used to apply [[nodiscard]] to entities not
 // specified as such as an extension.
-#  if defined(_LIBCPP_ENABLE_NODISCARD) && !defined(_LIBCPP_DISABLE_NODISCARD_EXT)
+#  if !defined(_LIBCPP_DISABLE_NODISCARD) && !defined(_LIBCPP_DISABLE_NODISCARD_EXT)
 #    define _LIBCPP_NODISCARD_EXT _LIBCPP_NODISCARD
 #  else
 #    define _LIBCPP_NODISCARD_EXT
 #  endif
 
-#  if !defined(_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17) && (_LIBCPP_STD_VER > 17 || defined(_LIBCPP_ENABLE_NODISCARD))
+#  if !defined(_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17) && (_LIBCPP_STD_VER > 17 || !defined(_LIBCPP_DISABLE_NODISCARD))
 #    define _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_NODISCARD
 #  else
 #    define _LIBCPP_NODISCARD_AFTER_CXX17
 #  endif
 
 #  if __has_attribute(no_destroy)
 #    define _LIBCPP_NO_DESTROY __attribute__((__no_destroy__))
 #  else
 #    define _LIBCPP_NO_DESTROY
 #  endif
 

libcxx/test/libcxx/algorithms/callable.verify.cpp

struct S {
     int i;
 
     S(int i_) : i(i_) {}
 
     bool compare(const S&) const;
   };
 
   S a[] = {1, 2, 3, 4};
-  std::lower_bound(a, a + 4, 0, &S::compare); // expected-error@*:* {{The comparator has to be callable}}
-  std::minmax({S{1}}, &S::compare); // expected-error@*:* {{The comparator has to be callable}}
-  std::minmax_element(a, a + 4, &S::compare); // expected-error@*:* {{The comparator has to be callable}}
+  (void) std::lower_bound(a, a + 4, 0, &S::compare); // expected-error@*:* {{The comparator has to be callable}}
+  (void) std::minmax({S{1}}, &S::compare); // expected-error@*:* {{The comparator has to be callable}}
+  (void) std::minmax_element(a, a + 4, &S::compare); // expected-error@*:* {{The comparator has to be callable}}
 }

libcxx/test/libcxx/diagnostics/enable_nodiscard.verify.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // Test that _LIBCPP_NODISCARD_EXT and _LIBCPP_NODISCARD_AFTER_CXX17 are defined
 // to the appropriate warning-generating attribute when _LIBCPP_ENABLE_NODISCARD
 // is explicitly provided.
 
 // UNSUPPORTED: c++03
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_NODISCARD
-
 #include <__config>
 
 _LIBCPP_NODISCARD_EXT int foo() { return 42; }
 _LIBCPP_NODISCARD_AFTER_CXX17 int bar() { return 42; }
 
 int main(int, char**) {
   foo(); // expected-warning-re {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
   bar(); // expected-warning-re {{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
   (void)foo(); // OK. void casts disable the diagnostic.
   (void)bar();
 
   return 0;
 }

libcxx/test/libcxx/diagnostics/enable_nodiscard_disable_after_cxx17.verify.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
 // Test that _LIBCPP_DISABLE_NODISCARD_EXT only disables _LIBCPP_NODISCARD_EXT
 // and not _LIBCPP_NODISCARD_AFTER_CXX17.
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_NODISCARD
 // ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17
 #include <__config>
 
 
 _LIBCPP_NODISCARD_EXT int foo() { return 42; }
 _LIBCPP_NODISCARD_AFTER_CXX17 int bar() { return 42; }
 
 int main(int, char**) {
   foo(); // expected-warning-re{{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
   bar(); // OK.
   (void)foo(); // OK.
 
   return 0;
 }

libcxx/test/libcxx/diagnostics/enable_nodiscard_disable_nodiscard_ext.verify.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_NODISCARD
 // ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_NODISCARD_EXT
 #include <__config>
 
 
 _LIBCPP_NODISCARD_EXT int foo() { return 42; }
 _LIBCPP_NODISCARD_AFTER_CXX17 int bar() { return 42; }
 
 int main(int, char**) {
   bar(); // expected-warning-re{{ignoring return value of function declared with {{'nodiscard'|warn_unused_result}} attribute}}
   foo(); // OK.
   (void)bar(); // OK.
 
   return 0;
 }

libcxx/test/libcxx/diagnostics/nodiscard.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-// Test that _LIBCPP_NODISCARD_EXT is not defined to [[nodiscard]] unless
-// explicitly enabled by _LIBCPP_ENABLE_NODISCARD
+// Test that _LIBCPP_NODISCARD_EXT is defined empty if _LIBCPP_DISABLE_NODISCARD_EXT is defined
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_NODISCARD
 
 #include <__config>
 
 #include "test_macros.h"
 
 _LIBCPP_NODISCARD_EXT int foo() { return 42; }
 
 int main(int, char**) {
   foo(); // OK.
 
   return 0;
 }

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

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-// Test that entities declared [[nodiscard]] as at extension by libc++, are
-// only actually declared such when _LIBCPP_ENABLE_NODISCARD is specified.
+// Test that entities declared [[nodiscard]] as at extension by libc++ are
+// not marked as such when _LIBCPP_DISABLE_NODISCARD is specified.
 
 // This test intentionally leaks memory, so it is unsupported under ASAN.
 // UNSUPPORTED: asan
 
 // 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.
 
 // Disable any builtin recognition of std::* in the compiler, that might also
 // trigger -Wunused-value warnings.
 // ADDITIONAL_COMPILE_FLAGS: -fno-builtin
 
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_NODISCARD
 // ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
 
 #include <algorithm>
 #include <bit> // bit_cast
 #include <cstddef> // to_integer
 #include <functional> // identity
 #include <iterator>
 #include <memory>

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

 
 // Test that entities declared [[nodiscard]] as an extension by libc++, are
 // only actually declared such when _LIBCPP_ENABLE_NODISCARD is specified.
 
 // 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
 // ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
 
 #include <algorithm>
 #include <bit> // bit_cast
 #include <cstddef> // to_integer
 #include <functional> // identity
 #include <iterator>
 #include <memory>

libcxx/test/libcxx/thread/thread.lock/thread.lock.guard/nodiscard.verify.cpp

 // template <class Mutex> class lock_guard;
 
 // [[nodiscard]] explicit lock_guard(mutex_type& m);
 // [[nodiscard]] lock_guard(mutex_type& m, adopt_lock_t);
 
 // Test that we properly apply [[nodiscard]] to lock_guard's constructors,
 // which is a libc++ extension.
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_NODISCARD
-
 #include <mutex>
 
 int main(int, char**) {
     std::mutex m;
     std::lock_guard<std::mutex>{m}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
     std::lock_guard<std::mutex>{m, std::adopt_lock}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
     return 0;
 }

libcxx/test/std/algorithms/alg.sorting/alg.min.max/requires_forward_iterator.fail.cpp

 #include "test_iterators.h"
 
 int main(int, char**) {
   int arr[] = {1, 2, 3};
   const int *b = std::begin(arr), *e = std::end(arr);
   typedef cpp17_input_iterator<const int*> Iter;
   {
     // expected-error@*:* {{"std::min_element requires a ForwardIterator"}}
-    std::min_element(Iter(b), Iter(e));
+    (void) std::min_element(Iter(b), Iter(e));
   }
   {
     // expected-error@*:* {{"std::max_element requires a ForwardIterator"}}
-    std::max_element(Iter(b), Iter(e));
+    (void) std::max_element(Iter(b), Iter(e));
   }
   {
     // expected-error@*:* {{"std::minmax_element requires a ForwardIterator"}}
-    std::minmax_element(Iter(b), Iter(e));
+    (void) std::minmax_element(Iter(b), Iter(e));
   }
 
 
   return 0;
 }