[mlpack] [mlpack/mlpack] Added copy constructor, move constructor and copy assignment operator. (#826)

[Ryan Curtin]

rcurtin commented on this pull request.

Thanks for adding the tests.  There are still a couple issues to take care of before we can merge this, though.

> +    ratio = obj.ratio;
+    logVolume = obj.logVolume;
+    bucketTag = obj.bucketTag;
+    alphaUpper = obj.alphaUpper;
+
+    //Free the space allocated
+    delete left;
+    delete right;
+
+    //Allocate space
+    left = new DTree;
+    right = new DTree;
+
+    //Copying the children 
+    left = obj.left;
+    right = obj.right;

Couldn't you simplify this to `left = new DTree(*obj.left);` and the same for the right?

> +    start(obj.start),
+    end(obj.end),
+    maxVals(obj.maxVals),
+    minVals(obj.minVals),
+    splitDim(obj.splitDim),
+    splitValue(obj.splitValue),
+    logNegError(obj.logNegError),
+    subtreeLeavesLogNegError(obj.subtreeLeavesLogNegError),
+    subtreeLeaves(obj.subtreeLeaves),
+    root(obj.root),
+    ratio(obj.ratio),
+    logVolume(obj.logVolume),
+    bucketTag(obj.bucketTag),
+    alphaUpper(obj.alphaUpper),
+    left(obj.left),
+    right(obj.right)

There are some unnecessary copies here, please use `std::move()` when applicable.

> +	if (this != &obj){  
+	//Copying the values from obj 
+    start = obj.start;
+    end = obj.end;
+    maxVals = obj.maxVals;
+    minVals = obj.minVals;
+    splitDim = obj.splitDim;
+    splitValue = obj.splitValue;
+    logNegError = obj.logNegError;
+    subtreeLeavesLogNegError = obj.subtreeLeavesLogNegError;
+    subtreeLeaves = obj.subtreeLeaves;
+    root = obj.root;
+    ratio = obj.ratio;
+    logVolume = obj.logVolume;
+    bucketTag = obj.bucketTag;
+    alphaUpper = obj.alphaUpper;

There are also some unnecessary copies here.

> +  BOOST_REQUIRE_EQUAL(tree2.maxVals[2], 8);
+  BOOST_REQUIRE_EQUAL(tree2.minVals[2], 1);
+
+  //Checking values of DTree made using copy assignment operator
+  BOOST_REQUIRE_EQUAL(tree3.maxVals[0], 7);
+  BOOST_REQUIRE_EQUAL(tree3.minVals[0], 3);
+  BOOST_REQUIRE_EQUAL(tree3.maxVals[1], 7);
+  BOOST_REQUIRE_EQUAL(tree3.minVals[1], 0);
+  BOOST_REQUIRE_EQUAL(tree3.maxVals[2], 8);
+  BOOST_REQUIRE_EQUAL(tree3.minVals[2], 1);
+
+  //Checking children were safely copied
+  BOOST_REQUIRE(tree2.Left()==leftChild);
+  BOOST_REQUIRE(tree2.Right()==rightChild);
+  BOOST_REQUIRE(tree3.Left()==leftChild);
+  BOOST_REQUIRE(tree3.Right()==rightChild);

At this point `leftChild` and `rightChild` should be invalid pointers; I bet valgrind will call this an invalid memory access and in some cases this test will fail.

> +  DTree<arma::mat> tree2(std::move(*tree));
+
+  // Deleting original DTree
+  delete tree;
+
+  //Checking maxValues and minValues of DTree made using move constructor.
+  BOOST_REQUIRE_EQUAL(tree2.maxVals[0], 7);
+  BOOST_REQUIRE_EQUAL(tree2.minVals[0], 3);
+  BOOST_REQUIRE_EQUAL(tree2.maxVals[1], 7);
+  BOOST_REQUIRE_EQUAL(tree2.minVals[1], 0);
+  BOOST_REQUIRE_EQUAL(tree2.maxVals[2], 8);
+  BOOST_REQUIRE_EQUAL(tree2.minVals[2], 1);
+
+  //Checking children were safely moved
+  BOOST_REQUIRE(tree2.Left()==leftChild);
+  BOOST_REQUIRE(tree2.Right()==rightChild);

Same issue with `leftChild` and `rightChild` here.

>  
+template <typename MatType, typename TagType>
+DTree<MatType, TagType>& DTree<MatType, TagType>::operator=(const DTree<MatType, TagType>& obj)
+{
+    if (this != &obj){

I don't think these checks are necessary.  If that is true, the user is doing something pretty stupid and we shouldn't need to handle it.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/mlpack/mlpack/pull/826#pullrequestreview-13850113
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://knife.lugatgt.org/pipermail/mlpack/attachments/20161220/423d54ff/attachment-0001.html>