Spanning Trees: Overload instead of weird ownership logic
authorPhil Miller <mille121@illinois.edu>
Fri, 5 Mar 2010 00:37:42 +0000 (18:37 -0600)
committerPhil Miller <mille121@illinois.edu>
Fri, 5 Mar 2010 01:23:50 +0000 (19:23 -0600)
Spanning trees generated for multicasts, reductions, and so forth can
be generated with either a user-supplied strategy or a default one
(selected by a factory knowledgeable of machien characteristics and so
forth). The logic to do this was based on checking if the argument to
buildSpanningTree[Generation] was NULL, and constructing a strategy if
it was. This strategy then had to be deleted afterward only if it was
locally constructed, a somewhat clunky arrangement.

Instead, split the "use the default strategy" logic into an overloaded
function that doesn't take the pointer argument, and passes through to
the (now-simplified) original version. Incidentally, some conditionals
are replaced by compile-time overload resolution.

src/util/spanningTreeStrategy.h

index af67b91a7aa092a039605dc4c13cd4aa55029e50..9df4b5eb78a371d92ed3c5a0684c5aa56239c100 100644 (file)
@@ -120,14 +120,24 @@ template <typename Iterator,typename ValueType = typename std::iterator_traits<I
 class SpanningTreeStrategy;
 
 /// Builds one generation of the spanning tree given a container of vertices with the tree root as the first element in the container
+// Use a default strategy
 template <typename Iterator>
 SpanningTreeVertex* buildSpanningTreeGeneration
-(const Iterator firstVtx, const Iterator beyondLastVtx, const int maxBranches=2, SpanningTreeStrategy<Iterator> *bldr = 0);
+(const Iterator firstVtx, const Iterator beyondLastVtx, const int maxBranches=2);
+// Use the strategy provided by the caller
+template <typename Iterator>
+SpanningTreeVertex* buildSpanningTreeGeneration
+(const Iterator firstVtx, const Iterator beyondLastVtx, const int maxBranches, SpanningTreeStrategy<Iterator> *bldr);
 
 /// Builds the complete spanning tree given a container of vertices with the tree root as the first element in the container
+// Use a default strategy
+template <typename Iterator>
+void buildSpanningTree
+(const Iterator firstVtx, const Iterator beyondLastVtx, const int maxBranches=2);
+// Use the strategy provided by the caller
 template <typename Iterator>
 void buildSpanningTree
-(const Iterator firstVtx, const Iterator beyondLastVtx, const int maxBranches=2, SpanningTreeStrategy<Iterator> *bldr = 0);
+(const Iterator firstVtx, const Iterator beyondLastVtx, const int maxBranches, SpanningTreeStrategy<Iterator> *bldr);
 
 /// Tiny factory method that returns a tree construction strategy that it thinks is best (based on inputs, the machine's network topology info etc)
 template <typename Iterator>
@@ -181,33 +191,30 @@ inline SpanningTreeVertex* buildSpanningTreeGeneration(const Iterator firstVtx,
                                                        SpanningTreeStrategy<Iterator> *bldr
                                                       )
 {
-    SpanningTreeVertex *result = 0;
+    CkAssert(NULL != bldr);
 
     /// Validate input. Invalid inputs are not exceptions. They are just no-ops
     if (maxBranches < 1 || firstVtx == beyondLastVtx)
-        return (result = new SpanningTreeVertex() );
-
-    // Should the tree builder strategy object be deleted?
-    bool shouldDelete = false;
-
-    /// If no strategy is passed in, instantiate one
-    if (bldr == 0)
-    {
-        bldr = getSpanningTreeStrategy(firstVtx,beyondLastVtx,maxBranches);
-        /// and remember to delete it after you're done
-        shouldDelete = true;
-    }
-
-    /// Delegate the actual work
-    result = bldr->buildNextGen(firstVtx,beyondLastVtx,maxBranches);
-
-    if (shouldDelete) delete bldr;
+        return new SpanningTreeVertex();
+    else
+       /// Delegate the actual work
+       return bldr->buildNextGen(firstVtx,beyondLastVtx,maxBranches);
+}
+// Overload to automatically use the default strategy
+template <typename Iterator>
+inline SpanningTreeVertex* buildSpanningTreeGeneration(const Iterator firstVtx,
+                                                       const Iterator beyondLastVtx,
+                                                       const int maxBranches
+                                                      )
+{
+    SpanningTreeStrategy<Iterator> *bldr =
+       getSpanningTreeStrategy(firstVtx,beyondLastVtx,maxBranches);
+    SpanningTreeVertex *result = buildSpanningTreeGeneration(firstVtx, beyondLastVtx, maxBranches, bldr);
+    delete bldr;
     return result;
 }
 
 
-
-
 /// Nested namespace to prevent the implementation muck from polluting topo::
 namespace impl {
 
@@ -246,7 +253,7 @@ void buildSpanningTree(SpanningTreeVertex* dispatchTag,
 }
 
 } // end namespace impl
+
 
 
 
@@ -262,21 +269,23 @@ inline void buildSpanningTree(const Iterator firstVtx,
                               SpanningTreeStrategy<Iterator> *bldr
                              )
 {
-    // Should the tree builder strategy object be deleted?
-    bool shouldDelete = false;
-    /// If no strategy is passed in, instantiate one
-    if (bldr == 0)
-    {
-        bldr = getSpanningTreeStrategy(firstVtx,beyondLastVtx,maxBranches);
-        /// and remember to delete it after you're done
-        shouldDelete = true;
-    }
+    CkAssert(NULL != bldr);
     /// Create a tag
     typename std::iterator_traits<Iterator>::value_type *tag = 0;
     /// Delegate the work
     impl::buildSpanningTree(tag,firstVtx,beyondLastVtx,maxBranches,bldr);
-    /// Delete the builder if it was not passed in
-    if (shouldDelete) delete bldr;
+}
+// Overload to automatically use the default strategy
+template <typename Iterator>
+inline void buildSpanningTree(const Iterator firstVtx,
+                              const Iterator beyondLastVtx,
+                              const int maxBranches
+                             )
+{
+    SpanningTreeStrategy<Iterator> *bldr =
+       getSpanningTreeStrategy(firstVtx,beyondLastVtx,maxBranches);
+    buildSpanningTree(firstVtx, beyondLastVtx, maxBranches, bldr);
+    delete bldr;
 }
 
 } // end namespace topo