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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/string:4037
+template<class _CharT, class _Traits, class _Allocator>
+[[nodiscard]] _LIBCPP_INLINE_VISIBILITY
+bool
----------------
Mordante wrote:
> cjdb wrote:
> > 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.
> > This doesn't answer my question.
> True when I started my review these comments weren't there yet, so our messages crossed.
> 
> The problem isn't the compiler support. The problem is that this function isn't declared as `[[nodiscard]]` in the Standard. In that case we use `_LIBCPP_NOEXCEPT_EXT`. See https://libcxx.llvm.org/docs/UsingLibcxx.html#libc-extensions for more information. I hope that clarifies the issue.
> 
We seem to be going in circles. You're saying "we need to use `_LIBCPP_NOEXCEPT_EXT` because it's an extension and the user might want to disable it". I'm saying "they already can, with `-Wno-unused-result`: why do we need two ways to do this?", to which the collective reply seems to be "we need to use `_LIBCPP_NOEXCEPT_EXT` because it's an extension and the user might want to disable it".

This document (which I'd read before initially asking why) doesn't address my question either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80895