[libcxx-commits] [PATCH] D122011: [libc++][ranges] Add implicit conversion to bool test for ranges::find{, if, if_not}

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision as: var-const.
var-const added inline comments.


================
Comment at: libcxx/test/support/boolean_testable.h:44
+public:
+  // this shouldn't be explicit to make it easier to initlaizer inside arrays (which it almost always is)
+  constexpr StrictComparable(T value) : value_{value} {}
----------------
Nit: `s/initlaizer/initialize/` (or perhaps it should be something like `use as an initializer`?).


================
Comment at: libcxx/test/support/boolean_testable.h:22
+
+  friend constexpr BooleanTestable operator!=(const BooleanTestable& lhs, const BooleanTestable& rhs) {
+    return !(lhs == rhs);
----------------
philnik wrote:
> var-const wrote:
> > Do we need this class to work pre-C++20?
> No, but the `operator!=` only gets implicitly generated if the return value of `operator==` is `bool`. Things only compilers know :D.
Interesting -- might be worth adding a comment, actually (but very optional).


================
Comment at: libcxx/test/support/boolean_testable.h:31
+private:
+  bool value_;
+};
----------------
philnik wrote:
> var-const wrote:
> > Optional: `= false`?
> Why? This class only has a single constructor which always sets the value. In this case I think it's more confusing than helpful.
Personally, I find it easier because a) I don't need to read through the constructors to make sure this primitive is always initialized, and b) it can prevent bugs (which are easy to fix but still take some unnecessary time) when adding a default constructor later. But if you prefer the current state, please keep as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122011