[PATCH] D14686: Protect against overloaded comma in random_shuffle and improve tests
Eric Fiselier via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 30 03:31:55 PST 2016
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
More information about the cfe-commits
mailing list