[libcxx-commits] [PATCH] D100429: [libc++][ranges] Add range.cmp: equal_to, not_equal_to, less, etc.
Christopher Di Bella via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Apr 14 19:41:23 PDT 2021
cjdb 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;
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100429/new/
https://reviews.llvm.org/D100429
More information about the libcxx-commits
mailing list