[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:
> 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.


================
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();
----------------
cjdb wrote:
> 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?
> I'm confused. Didn't you just add this file?

No, I moved it from `libcxx/test/std/utilities/function.objects/comparisons/pointer_comparison_test_helper.h`.

> 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.

So, would you rather I only test ~8 pointers? Even then we'd add at least 48 lines to the file, and we'd be significantly reducing the coverage. I think in the case of any loop, really, there isn't much readability lost for that logic. If this were in a helper function, or in an overloaded class template, that might be a different story, but I think everyone here is able to flatten a loop mentally. 

Also, I'd like to point out that this test isn't necessarily suppose to be easily verifiable on inspection; think about it more as a stress test or something. It's testing these operators on lots of different pointers as `uintptr_t` and `void*`.  

Think about it like the concept tests: I'm fine if we want to have a lot of complexity to stress test the compiler with hundreds or thousands of "generated" templates/macros. But let's keep those separate, and have the other, easily verifiable tests as well. That's why this is in a different file too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100429