[libcxx-commits] [PATCH] D100429: [libc++][ranges] Add range.cmp: equal_to, not_equal_to, less, etc.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__detail/noexcept_helpers.h:25
+
+#define _LIBCPP_NOEXCEPT_RETURN(...)                                           \
+  noexcept(noexcept(__VA_ARGS__)) { return __VA_ARGS__; }
----------------
zoecarver wrote:
> We never get a chance to `#undef` this now but I think that's OK.
Yes, that part is okay.
Based on @tcanens' repeated observations that this macro is being misused to create the wrong noexcept-specifiers, though, I think we ought to put a moratorium on this kind of macro. It's not really saving us any work, in the sense that it's actually //slowing down// reviews by making us have to skip back and forth to compare the function body with the definition of the macro, and then find out that the macro is doing the wrong thing in this case, and so on. I think noexcept-clauses should just be written out in the usual way, and this macro should be retired.

Bonus: no more `__detail/` dumping-ground directory. Bonus: no more `noexcept_helpers.h` (plural name, only one "helper", helper doesn't help).


================
Comment at: libcxx/test/support/pointer_comparison_test_helper.h:12
+template <template <class> class CompareTemplate>
+void do_pointer_comparison_test() {
+  typedef CompareTemplate<int*> Compare;
----------------
tcanens wrote:
> If you want to test this sort of thing, I'd suggest something like the example from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78420#c0. "Comparison produces a total order when the built-in is required to" (which describes 3/4 of the test cases below) is not a particularly interesting property compared to "comparison produces a total order even when the built-in is not required to".
How about
```
  struct {
    int a, b;
  } local;
  int *a = &local.a;
  int *b = &local.b;
  int *c = nullptr;
  int *d = &local.a + 1;
  std::uintptr_t ua = reinterpret_cast<std::uintptr_t>(a);
  std::uintptr_t ub = reinterpret_cast<std::uintptr_t>(b);
  std::uintptr_t uc = reinterpret_cast<std::uintptr_t>(c);
  std::uintptr_t ud = reinterpret_cast<std::uintptr_t>(d);
  assert(comp(a, a) == ucomp(ua, ua));
  assert(comp(a, b) == ucomp(ua, ub));
  assert(comp(a, c) == ucomp(ua, uc));
  assert(comp(a, d) == ucomp(ua, ud));
  assert(comp(b, a) == ucomp(ub, ua));
  assert(comp(b, b) == ucomp(ub, ub));
  assert(comp(b, c) == ucomp(ub, uc));
  assert(comp(b, d) == ucomp(ub, ud));
  assert(comp(c, a) == ucomp(uc, ua));
  assert(comp(c, b) == ucomp(uc, ub));
  assert(comp(c, c) == ucomp(uc, uc));
  assert(comp(c, d) == ucomp(uc, ud));
  assert(comp(d, a) == ucomp(ud, ua));
  assert(comp(d, b) == ucomp(ud, ub));
  assert(comp(d, c) == ucomp(ud, uc));
  assert(comp(d, d) == ucomp(ud, ud));
```
I haven't thought hard about how to factor out the testing of `std::less<int*>` and `std::less<>` so that you can just call the function template twice with different args instead of repeating that entire 16-line array of tests for `vcomp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100429