[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,
----------------
cjdb wrote:
> 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.
"Cast-to-void" counts as "fixing their broken code" (since that is a code change). Customers who "can't" change their code (e.g. because it's third-party code) generally can't even go patching it to add casts-to-void; they have to find solutions that operate strictly within the build system. Passing `-Wno-unused-result` is a legit solution. I just worry that it might be too much of a blunt instrument — it means there's no way for customers to opt out of the new aggressive warnings without //also// opting out of the old warnings (which by definition they'd already considered harmless).

Also, note that by "adding a knob," I really mean just "not removing the knob we already have." This PR consists basically of two parts: (1) changing `__config`, and (2) `sed -i 's/_EXT//' include/*`. I'm basically suggesting that we do (1) but not (2). You're suggesting that we do (1) and (2) now, and then later consider reverting only the (2) part. At least let's commit this in two pieces — the part-(1) piece I bet we'll never even consider reverting, and the part-(2) piece that we might end up reverting. That way we don't have to do all this work twice.

Data point: I ran this patched libc++ against my employer's codebase (which includes a large swath of Boost); it did not detect any true or false positives. Which is a good sign: out of a sample size of 1, I found 0 customers complaining about the effect of this PR. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108080