[libcxx-commits] [PATCH] D80895: [libcxx] adds consistent comparison for `basic_string`

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/string:4037
+template<class _CharT, class _Traits, class _Allocator>
+[[nodiscard]] _LIBCPP_INLINE_VISIBILITY
+bool
----------------
cjdb wrote:
> cjdb wrote:
> > Mordante wrote:
> > > Quuxplusone wrote:
> > > > Either remove `[[nodiscard]]` (the easy path), or replace it with `_LIBCPP_NODISCARD_EXT` and put a libcxx/test/libcxx/ test on it.
> > > The `[[nodiscard]]` is an extension, can you the `_LIBCPP_NODISCARD_EXT` macro and add it to the list extensions.
> > It's still unclear to me why `_LIBCPP_NODISCARD_EXT` is desirable over an attribute users can already disable.
> This doesn't answer my question. Most attributes don't change the meaning of a program, so it doesn't make sense to me why this macro is useful at all. Unless I'm grepping wrong, we don't have a `_LIBCPP_NOEXCEPT_EXT` macro for when the standard doesn't put `noexcept` on things. Why is `[[nodiscard]]` any different? This is at least my fourth time asking this question without an answer; I'd appreciate one please.
This is C++20 mode, so we can probably assume that all compilers support `[[nodiscard]]`, however, and this is a more general rule of thumb, if we decide that it would be beneficial to put `[[nodiscard]]` somewhere, we should probably use `_LIBCPP_NODISCARD_ATTRIBUTE ` or `_LIBCPP_NOEXCEPT_EXT` because it works on more platforms (i.e., `[[clang::warn_unused_result]]` on earlier versions of clang).

For places where the standard doesn't specifically ask us to use nodiscard, I think for now at least, it still makes sense to use `_LIBCPP_NODISCARD_EXT` (we can have a discussion at some point about retiring this attribute, if you'd like). This allows users to disable `[[nodiscard]]` if it's causing problems, or they don't want to use it for some reason. We can argue about the merits of such a configuration, but it's something that we have in libc++, and we shouldn't pull the rug from under people, at least not without discussion first. 

In the future you can generally look at the definition of such macros to see why they're useful, etc. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80895