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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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());
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664