CODEWITHSUNDEEP
c++algorithmc++11shuffle
Michiel uit het Broek
Michiel uit het Broek

Reputation: 993

random_shuffle modifies shuffled objects (c++11)

I have a vector with Tour objects and want to shuffle them. Fortunate there is a function random_shuffle(). I print the objects before and after the shuffling, however, one field of the objects is not shuffled at all.

The first thing to come to mind is that the copy or move constructor is incorrect. After using a cout in both constructors I found out that the move constructor is used. Strangely enough the constructors seems right to me.

The code and the move constructor are given below. Recall, only one field is incorrect after the shuffling, namely d_penalty. Can anyone help me solving this error?

std::vector<Tour> tours;
// Fill vector with 4 tour objects

std::cout << "Print vector\n";
for (Tour const &tour: tours)
  std::cout << tour << std::endl;

std::cout << "Shuffle\n";
std::random_shuffle(tours.begin(), tours.end());

std::cout << "Print vector\n";
for (Tour const &tour: tours)
  std::cout << tour << std::endl;

The move constructor is defined as follows,

#include "tour.ih"

/**
 * Move constructor
 */

Tour::Tour(Tour &&other)
:
  d_capacity(other.d_capacity),
  d_demand(other.d_demand),
  d_feasible(other.d_feasible),
  d_length(other.d_length),
  d_time(other.d_time),
  d_penalty(other.d_penalty),
  d_tour(std::move(other.d_tour))
{ 
  cout << "Tour moved\n";
}

The class is defined as follows,

class Tour
{
  // Current information about the tour
  int    d_capacity;    // Remaining capacity of the truck 
  int    d_demand;      // Total demand of all customers in the tour
  bool   d_feasible;    // Is the tour feasible in capacity
  double d_length;      // Length of the current tour
  double d_time;        // Driving time plus serving time
  double d_penalty;     // The penalty for infeasibility

  // The tour itself
  std::vector<City const *> d_tour;

  public:
    // Other functions

  private:
    // More functions
};

Upvotes: 0

Views: 440

Answers (1)

T.C.
T.C.

Reputation: 137315

std::random_shuffle swaps the elements. The default swap, implemented by std::swap, uses both move construction and move assignment.

If you don't have a move assignment operator, then the copy assignment operator would be used instead. Since your move constructor does handle d_penalty correctly, it sounds like that your copy assignment operator isn't implemented properly.

In general, a class that can benefit from move semantics should have both a move constructor and a move assignment operator. Where possible, the special member function should be defined as = default;.

Also, std::random_shuffle is deprecated in C++14 and removed in C++17; you should use std::shuffle and a URNG in the <random> header such as std::mt19937.

Upvotes: 3

Related Questions