[PATCH] D14686: Protect against overloaded comma in random_shuffle and improve tests

[Marshall Clow via cfe-commits]

mclow.lists added inline comments.

================
Comment at: test/std/algorithms/alg.modifying.operations/alg.random.shuffle/random_shuffle.pass.cpp:35
@@ -30,1 +34,3 @@
 }
+
+template <class Iter>
----------------
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.



Repository:
  rL LLVM

http://reviews.llvm.org/D14686