[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 requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/support/pointer_comparison_test_helper.h:22-23
+  const std::size_t test_size = 100;
+  for (size_t i = 0; i < test_size; ++i)
+    pointers.push_back(std::shared_ptr<T>(new T()));
+  Compare comp;
----------------
cjdb wrote:
> zoecarver wrote:
> > cjdb wrote:
> > > This can be made declarative using `std::generate_n`, and `std::make_shared<T>()` will make it clearer that there's no memory leaks.
> > Good suggestion, thanks. Not sure how it makes it clearer that there are no leaks, but it does make it much more readable, so I'll incorporate it.
> Whenever I see naked `new`, my eyes scan for a corresponding delete. It's not till after I can't find `delete` that my brain realises one might be constructing a smart pointer!
+1 on "no raw new and delete." However, -1 on writing `std::make_shared<T>` without calling it (i.e. relying on conversion to function pointer); I'm not sure that's conforming with SD-8 and it's certainly not in the //spirit// of SD-8. To get the same behavior simpler, I'd suggest
```
for (auto& ptr : pointers) {
    ptr = std::make_shared<T>();
}
```
However, I also don't understand why you need shared ownership of `T` objects here at all. Am I correct that all you're doing with these various heap-allocated objects is comparing their addresses over and over? And it doesn't matter what `T` is at all? This feels mildly silly. I would suggest simply making an array
```
int a[2];
int *lhs = &a[0];
int *rhs = &a[1];
auto lhs_uint = reinterpret_cast<std::uintptr_t>(lhs);
auto rhs_uint = reinterpret_cast<std::uintptr_t>(rhs);
assert(comp(lhs, rhs) == ucomp(lhs_uint, rhs_uint));
assert(vcomp(lhs, rhs) == ucomp(lhs_uint, rhs_uint));
```
Bonus: try with `int *ths = nullptr;` as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100429