[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 accepted this revision.
cjdb added a comment.

LGTM



================
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;
----------------
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!


================
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:
> > 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.
Ah, it's stress testing, not correctness testing. Carry on then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100429