[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