EricWF accepted this revision. EricWF added a comment. This revision is now accepted and ready to land.
@gribozavr The tests have changed a bunch since this patch was created. I took the liberty of re-merging the tests. You can find the updated patch here: https://gist.github.com/EricWF/a69933b79adf8cd61f1daa68c633cc03 Feel free to commit with the new tests. ================ Comment at: test/std/algorithms/alg.modifying.operations/alg.random.shuffle/random_shuffle.pass.cpp:35 } + +template <class Iter> ---------------- mclow.lists wrote: > gribozavr wrote: > > gribozavr wrote: > > > mclow.lists wrote: > > > > This is not how I would rewrite this test. > > > > I would write a routine that took two "iterators", and called > > > > `random_shuffle`, and then checked for the desired results. > > > > > > > > Then call it with pointers. and RA iters, etc. > > > > for example: > > > > > > > > template <Class Iter> > > > > void test(Iter first, Iter last, Iter resFirst, Iter resLast); > > > > > > > > test(nullptr, nullptr, nullptr, nullptr); > > > > int source[] = {1, 2, 3, 4}; > > > > int res [] = {1, 2, 3, 4}; > > > > const unsigned size = sizeof(source)/sizeof(source[0]); > > > > > > > > test(source, source + size, res, res+size); > > > > test(random_access_iterator<int*>(source) .... ); > > > > > > > > > > > I actually thought about this, and it is hard to rewrite it like that for > > > two reasons. First, `random_shuffle` mutates the elements, so one needs > > > to restore the original sequence between calls to `test()` (otherwise it > > > is not obvious that it was mutated). Second, this overload of > > > `random_shuffle` takes randomness from global state, so one can't just > > > specify one expected result in the test. That's why I first check for > > > the initial shuffle exactly, and then only check that the output is a > > > permutation of input. > > > > > Ping? > Which is why I think that your use of `is_permutation` is good below. > > What do we want to know after calling `random_shuffle`? > 1) The same items are in the range, only in a different order. (right? Can it > ever "do nothing") > 2) Nothing else is changed. > > If you have two collections with the same contents, you should be able to > shuffle one over and over, and call `is_permutation` to compare the two after > each call. > > Tests that have duplicates are good, too. > [ Yes, I know that your bug report (and fix!) are very localized, but these > tests are really lacking ] > > If you don't want to do this, I'll rewrite these tests sometime in the next > couple weeks, and then we can revisit your patch. > I think the tests as written are an improvement over the current tests, and I think @gribozavr addressed most of @mclow.lists points above pretty well. @mclow.lists Feel free to rewrite the tests once this has been committed. Repository: rL LLVM https://reviews.llvm.org/D14686 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
