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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


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



================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/string_pointer.pass.cpp:14
+// template<class charT, class traits, class Allocator>
+//   auto operator<=>(const basic_string<charT,traits,Allocator>& lhs, const charT* rhs);
+
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Please use `bool`.
> No, `operator<=>` isn't `bool` though! It's one of the ordering types, and apparently depends on `_Traits` (so even changing `auto` to `std::strong_ordering` wouldn't be right). I think `auto` is fine.
Good point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80895