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

[Christopher Di Bella via Phabricator via libcxx-commits]

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;
----------------
This can be made declarative using `std::generate_n`, and `std::make_shared<T>()` will make it clearer that there's no memory leaks.


================
Comment at: libcxx/test/support/pointer_comparison_test_helper.h:27-28
+  VoidCompare vcomp;
+  for (size_t i = 0; i < test_size; ++i) {
+    for (size_t j = 0; j < test_size; ++j) {
+      T* lhs = pointers[i].get();
----------------
zoecarver wrote:
> cjdb wrote:
> > Do we need to have loops in tests?
> I'd rather not update these tests. Also, I don't think this (albeit nested) loop is adding too much time.
Wanting to remove loops from tests isn't to do with run-time; it's to do with knowing the test is correct on inspection. The less logic that's in a test, the easier it is to verify that the test itself doesn't have a bug in it.

> I'd rather not update these tests

I'm confused. Didn't you just add this file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100429