[libcxx-commits] [PATCH] D108080: [libcxx] simplifies nodiscard by permanently enabling the attribute...

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/docs/UsingLibcxx.rst:274-281
+For this reason libc++ provides an extension that does just that! The extended
+applications of ``[[nodiscard]]`` take 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,
----------------
This PR seems acceptable to me. (I think it's likely better than the status quo, because it's simpler.) However, I'd still prefer to see libc++ distinguish the "conforming implementation" parts from the "non-conforming" parts. After all, this is supposed to be a valid C++ program, right?
```
#include <utility>
int main() { std::move(1); }
```
With this PR, libc++ will refuse to compile that program, so libc++ won't technically be conforming anymore. This //could// piss off customers with big old codebases. Whenever we do an API-breaking change, it's useful to give customers a way to opt out of it.

So, I counterpropose that we still do //most// of this simplification, but:
- Continue to mark the extensions with `_LIBCPP_NODISCARD_EXT` instead of `_LIBCPP_NODISCARD`
- Make `__config` look like this:
```
#if __has_cpp_attribute(nodiscard) || defined(_LIBCPP_COMPILER_MSVC)
#  define _LIBCPP_NODISCARD [[nodiscard]]
#elif defined(_LIBCPP_COMPILER_CLANG_BASED) && !defined(_LIBCPP_CXX03_LANG)
#  define _LIBCPP_NODISCARD [[clang::warn_unused_result]]
#else
// 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

#if defined(_LIBCPP_DISABLE_NODISCARD_EXT) && _LIBCPP_DISABLE_NODISCARD_EXT
#  define _LIBCPP_NODISCARD_EXT
#else
#  define _LIBCPP_NODISCARD_EXT _LIBCPP_NODISCARD
#endif
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108080