Moving an object from one vector to another in C++ (LEMON library)

Question

I am trying to use the LEMON library and I am running into an issue with an algorithm that I am trying to implement. The idea of this algorithm is that there is a vector of vectors of nodes and I want to move the nodes around to different vectors (color classes) with certain restrictions. Here is the code that implements my algorithm:

bool moveColor() {
  // pick the smallest color class
  sort(colors.begin(), colors.end(), vectorSort);
  vector< vector< ListGraph::Node > >::iterator smallestColor = colors.begin();

  // shuffle this class
  random_shuffle(smallestColor->begin(), smallestColor->end());

  // try to move any of the nodes to any bigger class
  bool foundNode = false;
  vector< ListGraph::Node >::iterator movingNode;
  vector< vector< ListGraph::Node > >::iterator destinationClass;
  for (movingNode = smallestColor->begin(); movingNode != smallestColor->end() && !foundNode; ++movingNode) {
    for (destinationClass = colors.begin()+1; destinationClass != colors.end() && !foundNode; ++destinationClass) {
      ListGraph::NodeMap<bool> filter(g, false);
      vector< ListGraph::Node >::iterator classNode;
      for (classNode = destinationClass->begin(); classNode != destinationClass->end(); ++classNode) {
        filter[*classNode] = true;
      }
      filter[*movingNode] = true;
      FilterNodes<ListGraph> subgraph(g, filter);
      if (acyclic(subgraph)) {
        foundNode = true;
      }
    }
  }

  // if a movable node was found, move it. otherwise return false
  if (foundNode) {
    destinationClass->push_back(*movingNode);
    smallestColor->erase(movingNode);
    if (smallestColor->empty()) {
      colors.erase(smallestColor);
    }
    return true;
  }
  return false;
}

This function is called in main in a loop until a node was unable to be moved. The main issue I am having with the code is the section that actually moves a node from one vector to another:

if (foundNode) {
  destinationClass->push_back(*movingNode);
  smallestColor->erase(movingNode);
  if (smallestColor->empty()) {
    colors.erase(smallestColor);
  }
  return true;
}

This part doesn't seem to work because I think the node is being deconstructed when the erase function is being called. Here is a sample of the node ids before and after this function was called:

NEW ROUND
1 
3 
0 
5 2 
7 6 4 
NEW ROUND
3 
0 32701 
5 2 
7 6 4 

As you can see, the id for the node that was moved is not correct (32701 instead of 1). Any help is appreciated.

Answer 1

The issue is actually here:

if (acyclic(subgraph)) {
    foundNode = true;
}

When you locate the node, you don't break out of for loop, which makes movingNode iterator advance to next position in vector (smallestColor->end() in this case) before checking for !foundNode condition. In this case you have to break twice, as you are breaking from nested loop.

Optionally, you can store iterator that match criteria in separate variable to operate on outside the loop (different that one iterating over in for loop).

About the snippet you highlighted as potential source of issue: destinationClass->push_back(*movingNode); is putting into destinationClass a copy of Node that movingNode iterator points to. That mean, copy constructor of Node is called. As Node has no user-defined copy constructor, compiler auto-generates one, that copy value of all members, so id value should be copied (of course if correct iterator is used for push_back). Of course original copy of node you are relocating is destructed with erase, but this does not affect new copy.