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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb 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,
----------------
Quuxplusone wrote:
> 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
> ```
> After all, this is supposed to be a valid C++ program, right?
> ```
> #include <utility>
> int main() { std::move(1); }
> ```

This is also a conforming C++20 program.

```
#include <vector>

int main()
{
  auto v = std::vector{0, 1, 2};
  v.empty(); // [[nodiscard]] as of C++20
}
```

> With this PR, libc++ will refuse to compile that program, so libc++ won't technically be conforming anymore.

Since `[[nodiscard]]` has no normative effect, both of our examples are technically conforming, with and without this patch.

> Whenever we do an API-breaking change, it's useful to give customers a way to opt out of it.

If a user doesn't want to/can't fix their broken code, there are still two ways they can opt out:

* Cast-to-void
* Enable `-Wno-unused-result`

> This //could// piss off customers with big old codebases.

I don't think we should hedge on the //possibility// of someone getting upset with libc++ because we now flag their buggy/questionable code (see my commit message). Let's apply it as-is, and if there are a substantial number of complaints, we can consider adding a libc++-specific knob then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108080