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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 4 inline comments as done.
cjdb added inline comments.


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


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/pointer_string.pass.cpp:31
+
+int main(int, char**)
+{
----------------
Mordante wrote:
> Can you turn the test into a templated test and test with all allowed string types? 
> The support header `make_string.h` has some macros that will be useful for these tests.
Not sure I understand what you're asking for, but let's take this up on Discord.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/pointer_string.pass.cpp:14
+// template<class charT, class traits, class Allocator>
+//   bool operator<=>(const charT* lhs, const basic_string<charT,traits,Allocator>& rhs);
+
----------------
Mordante wrote:
> curdeius wrote:
> > `bool`?
> Please address the comment above.
@curdeius was asking why it was `bool`, because it shouldn't be, so I've already addressed it.


================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string_opcmp/string_string.pass.cpp:12
+// <string>
+
+// we get this comparison "for free" because the string implicitly converts to the string_view
----------------
Mordante wrote:
> Please add the function being tested.
There isn't a function that's being tested. See the comment below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80895