[libc++] Consolidate the different [[nodiscard]] configuration options into a single one

[Mordante]

I wonder whether we want to take this approach. We've had patches in the past to remove this feature and make all [[nodiscard]] extensions always enabled. MSVC STL already does this and there have been done some tests on Google and Apple codebases. (I don't recall whether the results have been posted.)

How about going in that direction instead? probably we should then do this shortly after LLVM 15 is branched.

libcxx/docs/ReleaseNotes.rst

55

I feel this wording can use a bit of polish. I have a hard time to understand what this means.

libcxx/include/__config

869

Do we need two different macros or can they be combined? It doesn't need to be done in this patch.

[philnik]

In D129054#3630743, @Mordante wrote:

I wonder whether we want to take this approach. We've had patches in the past to remove this feature and make all [[nodiscard]] extensions always enabled. MSVC STL already does this and there have been done some tests on Google and Apple codebases. (I don't recall whether the results have been posted.)

How about going in that direction instead? probably we should then do this shortly after LLVM 15 is branched.

That's what I want to do in D128267, but there @ldionne requested that the different configuration macros should be consolidated into one, which I'm doing in this patch. I think these are two separate issues, so I wanted to land them separately.

[Mordante]

In D129054#3631115, @philnik wrote:

In D129054#3630743, @Mordante wrote:

I wonder whether we want to take this approach. We've had patches in the past to remove this feature and make all [[nodiscard]] extensions always enabled. MSVC STL already does this and there have been done some tests on Google and Apple codebases. (I don't recall whether the results have been posted.)

How about going in that direction instead? probably we should then do this shortly after LLVM 15 is branched.

That's what I want to do in D128267, but there @ldionne requested that the different configuration macros should be consolidated into one, which I'm doing in this patch. I think these are two separate issues, so I wanted to land them separately.

I missed that patch, thanks for the information. In that case consider my comment above as resolved.

[ldionne]

This change will require some updates to the documentation, please grep for _LIBCPP_ENABLE_NODISCARD and _LIBCPP_DISABLE_NODISCARD_EXT.

I think we are on the same page in essence, but this patch does not 100% reflect that. I think we should remove all of our various nodiscard-related settings in favour of a single _LIBCPP_DISABLE_NODISCARD_EXT, which would be enabled by default. Then, in D128267, we can flip the default to _LIBCPP_DISABLE_NODISCARD_EXT being disabled by default (i.e. nodiscard extensions being enabled by default).

libcxx/include/__config

862

[philnik]

• Address comments

libcxx/include/__config

869

IIUC we still want to distinguish between standards-mandated applications and extensions.

[ldionne]

This LGTM, but can you please ping me on Discord for a quick look once CI is passing?

IMO this is a nice simplification, and I can hardly imagine users wanting to apply some extended applications of nodiscard, but not all of them. So the flexibility we provided didn't pull its weight, IMO.

[philnik]

• Try to fix CI

[philnik]

• Fix CI

[thakis]

We (used to) use _LIBCPP_ENABLE_NODISCARD. I only happened to see this change here by coincidence.

It would be nice if there was a

#ifdef _LIBCPP_ENABLE_NODISCARD
#error _LIBCPP_ENABLE_NODISCARD no longer has an effect, define _LIBCPP_ENABLE_NODISCARD_EXT
#endif

in some central header to inform people in a similar situation.

[thakis]

(Also, libcxx/test/libcxx/diagnostics/nodiscard.pass.cpp still mentions _LIBCPP_ENABLE_NODISCARD in a comment.)

[philnik]

@thakis I think your comments should be addressed by D128267.

[thakis]

In D129054#3762104, @philnik wrote:

@thakis I think your comments should be addressed by D128267.

That's cool, but there still was (is) a Window where [[nodiscard]] disappeared for people who define _LIBCPP_ENABLE_NODISCARD.

In practice, it looks like nothing regressed for us in the 1 week it took us to notice this, but if some hard-to-update dependency had added code that ignored a nodiscard, this would've been pretty painful to deal with.

So I still think it'd be nice if that #error was there until D128267 lands.

libcxx/docs/ReleaseNotes.rst

 -------------------------
 
 Upcoming Deprecations and Removals
 ----------------------------------
 
 API Changes
 -----------
 - The comparison operators on ``thread::id`` are now defined as free-standing
   functions instead of as hidden friends, in conformance with the C++ standard.
   Also see `issue 56187 <https://github.com/llvm/llvm-project/issues/56187>`_.
 
+- ``_LIBCPP_ENABLE_NODISCARD`` and ``_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17`` are no longer respected.
+  Any standards-required ``[[nodiscard]]`` applications in C++20 are now always enabled. Any extended applications
+  can now be enabled by defining ``_LIBCPP_ENABLE_NODISCARD_EXT``.
+
 ABI Affecting Changes
 ---------------------
 
 Build System Changes
 --------------------

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.
-  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.
+**_LIBCPP_ENABLE_NODISCARD_EXT**:
+  This macro allows the library to apply ``[[nodiscard]]`` to entities as an extension.
+  See :ref:`Extended Applications of [[nodiscard]] <nodiscard extension>` for more information.
 
 **_LIBCPP_DISABLE_DEPRECATION_WARNINGS**:
   This macro disables warnings when using deprecated components. For example,
   using `std::auto_ptr` when compiling in C++11 mode will normally trigger a
   warning saying that `std::auto_ptr` is deprecated. If the macro is defined,
   no warning will be emitted. By default, this macro is not defined.
 
 C++17 Specific Configuration Macros

 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
+extension must be enabled by defining ``_LIBCPP_ENABLE_NODISCARD_EXT``. 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
-additional macros.
-
-Applications of the first form, which backport ``[[nodiscard]]`` from a newer
-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``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 This section lists all extended applications of ``[[nodiscard]]`` to entities
 which no dialect declares as such (See the second form described above).
 
 * ``adjacent_find``
 * ``all_of``

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_ENABLE_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 _LIBCPP_STD_VER > 17 || defined(_LIBCPP_ENABLE_NODISCARD_EXT)
 #    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

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/enable_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
 //
 //===----------------------------------------------------------------------===//
 
 // 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
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_NODISCARD_EXT
 
 #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/nodiscard_aftercxx17.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_AFTER_CXX17 is disabled whenever
-// _LIBCPP_DISABLE_NODISCARD_AFTER_CXX17 is defined by the user.
-
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17
-
-#include <__config>
-
-#include "test_macros.h"
-
-_LIBCPP_NODISCARD_AFTER_CXX17 int foo() { return 6; }
-
-int main(int, char**)
-{
-  foo();  // no error here!
-
-  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.
+// only actually declared such when _LIBCPP_ENABLE_NODISCARD_EXT 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 tested here and in nodiscard_extensions.verify.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_DEPRECATION_WARNINGS
 

libcxx/test/libcxx/diagnostics/nodiscard_extensions.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 entities declared [[nodiscard]] as an extension by libc++, are
-// only actually declared such when _LIBCPP_ENABLE_NODISCARD is specified.
+// only actually declared such when _LIBCPP_ENABLE_NODISCARD_EXT 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_ENABLE_NODISCARD_EXT
 // 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
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_NODISCARD_EXT
 
 #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;
 }