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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
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:
> Quuxplusone wrote:
> > 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.
> Alternatively (preferred): put `make_shared` in a lambda.
> 
> Alternatively, alternatively: we //are// the implementation and if we break ourselves, that's something we need to fix down the road.
Updated this to be a static size array. Good call. No need for the template/shared_ptr. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100429