Re: [Eliot-dev] [PATCH] Stop using std:rand() and use boost::Random. This allow us to create reproductible test cases.

[Olivier Teuliere]

Hi Antoine,

Sorry for the late review...
I added a few comments to your patch inline, but I have also a few generic
comments:
1) There is still one place where your Random class should be used: in
   the PlayedRack::shuffleNew() method (instead of the call to
   std::random_shuffle())
2) Most of the non regression tests are probably broken by this patch.
   We have to fix them, and it's quite a big work (we cannot simply
   replace the .ref file with the .run one, because it would not be
   testing the same thing at all in most cases)
3) Is there a guarantee that the algorithms used by Boost will be stable
   over time? For Eliot needs, we should always get the same sequence of
   numbers for a given algorithm and seed. Otherwise we will have to
   modify the tests regularly, which I really want to avoid...

On Thu, Jun 28, 2012 at 12:15 AM, Antoine VINCENT
<address@hidden> wrote:
> --- /dev/null
> +++ b/dic/random.cpp
> @@ -0,0 +1,78 @@
> +/*****************************************************************************
> + * Eliot
> + * Copyright (C) 2002-2009 Antoine Fraboulet & Olivier Teulière
> + * Authors: Antoine Fraboulet <antoine.fraboulet @@ free.fr>
> + *          Olivier Teulière  <ipkiss @@ gmail.com>

Hmmm, really? :)

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + 
> *****************************************************************************/
> +
> +#include "random.h"
> +
> +#include <ctime>            // std::time
> +#include <boost/random/uniform_real.hpp>
> +#include <boost/random/variate_generator.hpp>
> +#include <boost/random/linear_congruential.hpp>
> +
> +
> +// This is a typedef for a random number generator.
> +typedef boost::rand48 base_generator_type;
> +
> +class RandomGenerator
> +{
> +public:
> +    RandomGenerator(unsigned int iSeed) : rng(iSeed), uni_dist(rng, 
> boost::uniform_real<>(0, 1))
> +    {
> +    }
> +
> +    double generate() {
> +        return uni_dist();
> +    }
> +
> +private:
> +    // Use the boost random number generator
> +    base_generator_type rng;
> +    // Make a variate_generator OBJECT.
> +    boost::variate_generator<base_generator_type&,boost::uniform_real<> > 
> uni_dist;

Please prefix member variables with m_, and use camelCase for variables
and methods names.

> +};
> +
> +Random *Random::instance = 0;
> +
> +Random::Random(unsigned int iSeed)
> +{
> +    this->rdmgen = new RandomGenerator(iSeed);
> +}
> +
> +Random::~Random()
> +{
> +    delete this->rdmgen;
> +}
> +
> +Random *Random::getInstance(unsigned int iSeed = time(NULL))

I don't think you can define a default argument value in the cpp file,
it should be in the header instead. Anyway, I would prefer 2 different
methods:
 - initInstance() (or something like that), taking an unsigned argument
 - getInstance() without argument, to retrieve the existing instance
The idea is to really distinguish the initialization from later
operations and to avoid an unintentional initialization.

> +   {
> +      if (!Random::instance)
> +          Random::instance = new Random(iSeed);
> +      return Random::instance;
> +   }
> +
> +double Random::getRand() {

Please keep brackets on their own line.

> +    return Random::getInstance()->rdmgen->generate();
> +}
> +
> +
> +/* Actuel
> +int n = (int)((double)total * rand() / (RAND_MAX + 1.0));
> +int n = (int)(nbLettres * rand% => indice lettre
> +*/

??

> diff --git a/dic/random.h b/dic/random.h
> new file mode 100644
> index 0000000..c5e48a5
> --- /dev/null
> +++ b/dic/random.h
> @@ -0,0 +1,26 @@
> +#ifndef RANDOM_H
> +#define RANDOM_H
> +
> +class RandomGenerator;
> +
> +// TODO Destroy ?

Yes, I think it would be nice :)

> +class Random

Please add a logger to the Random class. It's practical to have one in
every class, when debugging.

> +{
> +public:
> +    static Random *getInstance(unsigned int iSeed);
> +
> +    static double getRand();

Why is this one static?

> +
> +private:
> +    ~Random();
> +    Random(unsigned int iSeed);
> +
> +    // Generator
> +    RandomGenerator *rdmgen;
> +
> +    // Singleton
> +    static Random *instance;
> +};
> +
> +#endif // RANDOM_H
> diff --git a/game/bag.cpp b/game/bag.cpp
> index e683cf2..889c7cc 100644
> --- a/game/bag.cpp
> +++ b/game/bag.cpp
> @@ -23,12 +23,13 @@
>
>  #include <string>
>  #include <cstdio>
> -#include <cstdlib> // For rand()
> +// #include <cstdlib> // For rand()

No need to keep the old code here...

>
>  #include <dic.h>
>  #include "bag.h"
>  #include "debug.h"
>  #include "encoding.h"
> +#include "random.h"
>
>
>  INIT_LOGGER(game, Bag);
> @@ -125,7 +126,10 @@ Tile Bag::selectRandomTile(unsigned int total,
>  {
>      ASSERT(total > 0, "Not enough tiles (of the requested kind) in the bag");
>
> -    int n = (int)((double)total * rand() / (RAND_MAX + 1.0));
> +    //int n = (int)((double)total * rand() / (RAND_MAX + 1.0));
> +    int n = (int)((double)total * Random::getRand());
> +    LOG_DEBUG("int generated : " << n);
> +

... and here

>      std::pair<Tile, int> p;
>      BOOST_FOREACH(p, m_tilesMap)
>      {
> diff --git a/qt/main_window.cpp b/qt/main_window.cpp
> index 8660d2c..419acae 100644
> --- a/qt/main_window.cpp
> +++ b/qt/main_window.cpp
> @@ -67,7 +67,7 @@
>  #include "dic_wizard.h"
>  #include "aux_window.h"
>  #include "qtcommon.h"
> -
> +#include "random.h"
>
>  INIT_LOGGER(qt, MainWindow);
>
> @@ -96,13 +96,8 @@ MainWindow::MainWindow(QWidget *iParent)
>      readSettings();
>
>      // Initialize the random numbers generator
> -    // Note: This must be done _after_ creating the QMenuBar object,
> -    // because on Gnome QMenuBar calls gconftool2, which for some reason
> -    // calls srand() internally...
> -    // This could be disabled using QApplication::setDesktopSettingsAware(),
> -    // but we would lose the desktop integration...
>      unsigned int val = time(NULL);
> -    srand(val);
> +    Random::getInstance(val);
>
>      // Make it easier to reproduce bugs
>      LOG_DEBUG("Rand seed: " << val);
> diff --git a/utils/eliottxt.cpp b/utils/eliottxt.cpp
> index 02a0274..ab25f1d 100644
> --- a/utils/eliottxt.cpp
> +++ b/utils/eliottxt.cpp
> @@ -52,6 +52,7 @@
>  #include "base_exception.h"
>  #include "settings.h"
>  #include "move.h"
> +#include "random.h"
>
>  class Game;
>
> @@ -950,7 +951,7 @@ int main(int argc, char *argv[])
>              seed = atoi(argv[2]);
>          else
>              seed = time(NULL);
> -        srand(seed);
> +        Random::getInstance(seed);
>          cerr << "Using seed: " << seed << endl;
>
>          mainLoop(dic);
> --
> 1.7.9.5

Best regards,
-- 
Olivier