Merge pull request #1983 from mcm001/bayestree-dtor-std-move

Reduce smart pointer copies in destructors

gtsam/base/timing.cpp

@@ -109,7 +109,7 @@ void TimingOutline::print(const std::string& outline) const {
 
 /* ************************************************************************* */
 void TimingOutline::printCsvHeader(bool addLineBreak) const {
-#ifdef GTSAM_USE_BOOST_FEATURES
+#if GTSAM_USE_BOOST_FEATURES
   // Order is (CPU time, number of times, wall time, time + children in seconds,
   // min time, max time)
   std::cout << label_ + " cpu time (s)" << "," << label_ + " #calls" << ","
@@ -134,7 +134,7 @@ void TimingOutline::printCsvHeader(bool addLineBreak) const {
 
 /* ************************************************************************* */
 void TimingOutline::printCsv(bool addLineBreak) const {
-#ifdef GTSAM_USE_BOOST_FEATURES
+#if GTSAM_USE_BOOST_FEATURES
   // Order is (CPU time, number of times, wall time, time + children in seconds,
   // min time, max time)
   std::cout << self() << "," << n_ << "," << wall() << "," << secs() << ","

gtsam/constrained/NonlinearConstraint.h

@@ -21,7 +21,7 @@
 #include <gtsam/inference/FactorGraph.h>
 #include <gtsam/inference/FactorGraph-inst.h>
 #include <gtsam/nonlinear/NonlinearFactor.h>
-#ifdef GTSAM_ENABLE_BOOST_SERIALIZATION
+#if GTSAM_ENABLE_BOOST_SERIALIZATION
 #include <boost/serialization/base_object.hpp>
 #endif
 
@@ -77,7 +77,7 @@ class GTSAM_EXPORT NonlinearConstraint : public NoiseModelFactor {
   }
 
  private:
-#ifdef GTSAM_ENABLE_BOOST_SERIALIZATION
+#if GTSAM_ENABLE_BOOST_SERIALIZATION
   /** Serialization function */
   friend class boost::serialization::access;
   template <class ARCHIVE>

gtsam/constrained/NonlinearEqualityConstraint.h

@@ -39,7 +39,7 @@ class GTSAM_EXPORT NonlinearEqualityConstraint : public NonlinearConstraint {
   virtual ~NonlinearEqualityConstraint() {}
 
  private:
-#ifdef GTSAM_ENABLE_BOOST_SERIALIZATION
+#if GTSAM_ENABLE_BOOST_SERIALIZATION
   /** Serialization function */
   friend class boost::serialization::access;
   template <class ARCHIVE>
@@ -85,7 +85,7 @@ class ExpressionEqualityConstraint : public NonlinearEqualityConstraint {
   }
 
  private:
-#ifdef GTSAM_ENABLE_BOOST_SERIALIZATION
+#if GTSAM_ENABLE_BOOST_SERIALIZATION
   /** Serialization function */
   friend class boost::serialization::access;
   template <class ARCHIVE>
@@ -131,7 +131,7 @@ class GTSAM_EXPORT ZeroCostConstraint : public NonlinearEqualityConstraint {
   }
 
  private:
-#ifdef GTSAM_ENABLE_BOOST_SERIALIZATION
+#if GTSAM_ENABLE_BOOST_SERIALIZATION
   /** Serialization function */
   friend class boost::serialization::access;
   template <class ARCHIVE>
@@ -166,7 +166,7 @@ class GTSAM_EXPORT NonlinearEqualityConstraints : public FactorGraph<NonlinearEq
   NonlinearFactorGraph penaltyGraph(const double mu = 1.0) const;
 
  private:
-#ifdef GTSAM_ENABLE_BOOST_SERIALIZATION
+#if GTSAM_ENABLE_BOOST_SERIALIZATION
   /** Serialization function */
   friend class boost::serialization::access;
   template <class ARCHIVE>

gtsam/constrained/NonlinearInequalityConstraint.h

@@ -61,7 +61,7 @@ class GTSAM_EXPORT NonlinearInequalityConstraint : public NonlinearConstraint {
   virtual NoiseModelFactor::shared_ptr penaltyFactorEquality(const double mu = 1.0) const;
 
  private:
-#ifdef GTSAM_ENABLE_BOOST_SERIALIZATION
+#if GTSAM_ENABLE_BOOST_SERIALIZATION
   /** Serialization function */
   friend class boost::serialization::access;
   template <class ARCHIVE>
@@ -129,7 +129,7 @@ class GTSAM_EXPORT ScalarExpressionInequalityConstraint : public NonlinearInequa
   }
 
  private:
-#ifdef GTSAM_ENABLE_BOOST_SERIALIZATION
+#if GTSAM_ENABLE_BOOST_SERIALIZATION
   /** Serialization function */
   friend class boost::serialization::access;
   template <class ARCHIVE>
@@ -169,7 +169,7 @@ class GTSAM_EXPORT NonlinearInequalityConstraints
                                           const double mu = 1.0) const;
 
  private:
-#ifdef GTSAM_ENABLE_BOOST_SERIALIZATION
+#if GTSAM_ENABLE_BOOST_SERIALIZATION
   /** Serialization function */
   friend class boost::serialization::access;
   template <class ARCHIVE>

gtsam/inference/BayesTree-inst.h

@@ -196,21 +196,20 @@ namespace gtsam {
     for (auto&& root: roots_) {
       std::queue<sharedClique> bfs_queue;
 
-      // first, move the root to the queue
-      bfs_queue.push(root);
-      root = nullptr; // now the root node is owned by the queue
+      // first, steal the root and move it to the queue. This invalidates root
+      bfs_queue.push(std::move(root));
 
       // do a BFS on the tree, for each node, add its children to the queue, and then delete it from the queue
       // So if the reference count of the node is 1, it will be deleted, and because its children are in the queue,
       // the deletion of the node will not trigger a recursive deletion of the children.
       while (!bfs_queue.empty()) {
-        // move the ownership of the front node from the queue to the current variable
-        auto current = bfs_queue.front();
+        // move the ownership of the front node from the queue to the current variable, invalidating the sharedClique at the front of the queue
+        auto current = std::move(bfs_queue.front());
         bfs_queue.pop();
 
         // add the children of the current node to the queue, so that the queue will also own the children nodes.
         for (auto child: current->children) {
-          bfs_queue.push(child);
+          bfs_queue.push(std::move(child));
         } // leaving the scope of current will decrease the reference count of the current node by 1, and if the reference count is 0,
           // the node will be deleted. Because the children are in the queue, the deletion of the node will not trigger a recursive
           // deletion of the children.

gtsam/inference/ClusterTree-inst.h

@@ -114,20 +114,20 @@ ClusterTree<GRAPH>::~ClusterTree() {
 
   for (auto&& root : roots_) {
     std::queue<sharedNode> bfs_queue;
-    // first, move the root to the queue
-    bfs_queue.push(root);
-    root = nullptr; // now the root node is owned by the queue
+
+    // first, steal the root and move it to the queue. This invalidates root
+    bfs_queue.push(std::move(root));
 
     // for each node iterated, if its reference count is 1, it will be deleted while its children are still in the queue.
     // so that the recursive deletion will not happen.
     while (!bfs_queue.empty()) {
-      // move the ownership of the front node from the queue to the current variable
-      auto node = bfs_queue.front();
+      // move the ownership of the front node from the queue to the current variable, invalidating the sharedClique at the front of the queue
+      auto node = std::move(bfs_queue.front());
       bfs_queue.pop();
 
       // add the children of the current node to the queue, so that the queue will also own the children nodes.
       for (auto child : node->children) {
-        bfs_queue.push(child);
+        bfs_queue.push(std::move(child));
       } // leaving the scope of current will decrease the reference count of the current node by 1, and if the reference count is 0,
         // the node will be deleted. Because the children are in the queue, the deletion of the node will not trigger a recursive
         // deletion of the children.

gtsam/inference/ClusterTree.h

@@ -166,9 +166,9 @@ class ClusterTree {
 
   /// @}
 
+ protected:
   ~ClusterTree();
 
- protected:
   /// @name Details
   /// @{
 

gtsam/inference/EliminationTree-inst.h

@@ -197,20 +197,19 @@ namespace gtsam {
     for (auto&& root : roots_) {
       std::queue<sharedNode> bfs_queue;
 
-      // first, move the root to the queue
-      bfs_queue.push(root);
-      root = nullptr; // now the root node is owned by the queue
+      // first, steal the root and move it to the queue. This invalidates root
+      bfs_queue.push(std::move(root));
 
       // for each node iterated, if its reference count is 1, it will be deleted while its children are still in the queue.
       // so that the recursive deletion will not happen.
       while (!bfs_queue.empty()) {
-        // move the ownership of the front node from the queue to the current variable
-        auto node = bfs_queue.front();
+        // move the ownership of the front node from the queue to the current variable, invalidating the sharedClique at the front of the queue
+        auto node = std::move(bfs_queue.front());
         bfs_queue.pop();
 
         // add the children of the current node to the queue, so that the queue will also own the children nodes.
         for (auto&& child : node->children) {
-          bfs_queue.push(child);
+          bfs_queue.push(std::move(child));
         } // leaving the scope of current will decrease the reference count of the current node by 1, and if the reference count is 0,
           // the node will be deleted. Because the children are in the queue, the deletion of the node will not trigger a recursive
           // deletion of the children.

gtsam/inference/EliminationTree.h

@@ -117,8 +117,10 @@ namespace gtsam {
 
     /// @}
 
-  public:
+  protected:
     ~EliminationTree(); 
+
+  public:
     /// @name Standard Interface
     /// @{