[libcxx-commits] [PATCH] D96664: [libc++][format] Implement formatters.

[Mark de Wever via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/test/std/utilities/format/format.functions/vformat_to.pass.cpp:36-41
+template <class CharT>
+bool operator==(const std::list<CharT>& lhs,
+                const std::basic_string<CharT>& rhs) {
+  return lhs.size() == rhs.size() &&
+         std::equal(lhs.begin(), lhs.end(), rhs.begin());
+}
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > Please don't provide `==`s for library types, not even in .cpp files. In this case your initial `size` check can be omitted anyway, because `std::equal` can do it: just change e.g. line 68 to
> > > ```
> > > assert(std::equal(out.begin(), out.end(), expected.begin(), expected.end()));
> > > ```
> > I'm not objecting against this change, but what's the reason you done like `operator==` for library types?
> Combination of two premises:
> (1) Users shouldn't add `operator==` for types they don't own, and users don't own STL types. (If someone filed a bug report and attached this code, saying "libc++ misbehaves if I add my own `operator==` between vector and string!"— well, we'd just close it as NOTABUG, right?)
> (2) The libc++ test suite should contain //only// such code as our users might //legitimately// write. Otherwise what would we really be testing here?— we'd be testing that libc++ works //if// the user provides an `operator==` between vector and string— but literally no real user ever does that, nor are they allowed to do it, so testing that configuration isn't meaningful. Or at least the test isn't as meaningful as it could be.
> 
> I suspect the controversial premise is more likely (2) than (1), but anyway I tried to explain both; and I think you'd agree that //if// you accept both (1) and (2), then it follows that the libc++ test suite shouldn't add extra `operator==`s for STL types.
Thanks for the information. I fully agree with (2). However I wasn't aware that (1) is a rule. I've seen operator overloading for standard library types quite often, for example `operator<<` for the stream objects.

Even if don't fully agree with (1) I still feel your solution of using a standard algorithm is better then writing the operator overload.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664