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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked an inline comment as done.
cjdb added inline comments.


================
Comment at: libcxx/include/string:4037
+template<class _CharT, class _Traits, class _Allocator>
+[[nodiscard]] _LIBCPP_INLINE_VISIBILITY
+bool
----------------
zoecarver wrote:
> 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. 
> 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).
> 

Aren't we now only supporting as far back as LLVM ${CURRENT_VERSION}-2? That would mean this patch only needs to worry about LLVM 11+, and this attribute has been supported since Clang 3.9.

> 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. 

This still doesn't address my original question. Given `-Wno-unused-result` turns off `[[nodiscard]]`, and is present on both Clang and GCC, I don't see why we want to have two ways to toggle the same thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80895