[libcxx-dev] Proposal to replace `_LIBCPP_NODISCARD_EXT` with `[[nodiscard]]`

[Louis Dionne via libcxx-dev]

Hi folks,

I was out for a bit, but now I'm back. I see there's been a lot of discussion about [[nodiscard]] lately (here and on Discord), and I think we should try to settle that story once and for all. My opinion has two aspects to it:

First, I think that we should apply `[[nodiscard]]` conservatively, only in the places where it is mandated by the Standard, or where we know almost for sure that it is a vexing bug hidden (like `std::vector::empty` or `ranges::empty`). Something like `std::vector::size`, for example, is not a good candidate for getting `[[nodiscard]]`. Yes, it doesn't make much sense to call `.size()` if you discard the result. However, it is not something that we have evidence of it causing bugs, unlike for `empty` or things like `lock_guard`'s constructor. Catching vexing cases like `.empty()` and `lock_guard` is the primary reason for adding the attribute - policing pretty much all functions that return non-`void` isn't.

If we were designing the language from scratch, I'd probably argue that all non-`void` functions should be `[[nodiscard]]` implicitly, and then we'd have an attribute to allow discarding the result instead. I think that would be far more ergonomic. But that's not the language we have, and I think we should try to work with it instead of starting to annotate almost every single function we write with an attribute. Annotating everything has a cost in readability, doesn't provide obvious value (I'm not convinced that annotating something like `.size()` will help catch any bug, ever), and will almost certainly cause issues for people trying to update to the new version of the library, in case they use some `[[nodiscard]]` functions in dubious but valid places. That brings us to the second point about extensions vs mandated `[[nodiscard]]`.

I believe that if we stick with the guideline above of only adding `[[nodiscard]]` to places that are truly vexing (and so we should basically consider it a bug that the Standard doesn't mandate `[[nodiscard]]` on those), the debate of whether to enable `[[nodiscard]]` extensions by default or not becomes pretty much moot. Indeed, if we only add `[[nodiscard]]` conservatively to places that are really catching bugs, we shouldn't trigger any false positives in user code. Hence, nobody should notice that we added these extensions, and if they do, then they'd be thankful that we've done so because we'd have caught a bug. For that reason, I believe that it's fine to just use `[[nodiscard]]` unconditionally (not guarded behind some sort of extension macro) if we stay conservative about where we use it. Furthermore, under that guideline, I think we should consider writing a paper to make it part of the Standard every time we add `[[nodiscard]]` as an extension.

So, my preference would be to review the current uses of `[[nodiscard]]` in the library (most of which have been added recently) and remove those that do not abide to the guideline above. Similarly, we could turn uses of `_LIBCPP_NODISCARD_EXT` into `_LIBCPP_NODISCARD` and update the documentation accordingly (alongside with a release note, but we should expect that change to never break anyone if we're being conservative).

Louis


> On May 13, 2021, at 13:47, Christopher Di Bella via libcxx-dev <libcxx-dev at lists.llvm.org> wrote:
> 
> Hi folks,
> 
> There's been a lot of requests for `[[nodiscard]]` to be replaced with `_LIBCPP_NODISCARD_EXT` in recent reviews where the standard doesn't explicitly declare an entity with the nodiscard attribute. After some investigation, I've discovered that this feature is confusing and error-prone. `_LIBCPP_NODISCARD_EXT` is opt-in, which requires users to know about its existence (I certainly didn't until reviewers started asking for it, and even then, didn't expect it to be opt-in), and then has three ways to opt out of its use: `_LIBCPP_DISABLE_NODISCARD_EXT` (global), `-Wno-unused-result` (global), and cast to `void` (local).
> 
> Since It's unclear why we want so many toggle options for this feature, this attribute doesn't change a user's program <http://eel.is/c++draft/dcl.attr.nodiscard>, and users already have two compiler-based opt-out mechanisms <https://godbolt.org/z/hsWo1v5W9> for all supported compilers and C++ standards, I think it's reasonable to remove this attribute in favour of the easier-to-use attribute itself.
> 
> Kind regards,
> 
> Christopher Di Bella
> _______________________________________________
> libcxx-dev mailing list
> libcxx-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/libcxx-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libcxx-dev/attachments/20210520/47a03d89/attachment.html>