[!72] Cleanup child and childStorage

Merge branch 'feature/remove_child_storage' into 'master'

ref:staging/dune-typetree\>

### Summary

Cleanup the child(),childStorage() node member functions and corresponding
free functions and removed type aliases ChildStorage and ConstChildStorage
from node implementations

### Details

-   The child-storage is in all node implementations a shared_ptr of the child
    type. This is even implicitly assumed in several methods, like setChild().
    So it does not make much sense to allow flexible child storage types.
-   The child-extraction methods `childStorage(node, i...)` are not
    consistently implemented like `child(node, i...)` and several type aliases
    or free functions are missing, so this MR cleans this up.
-   The member functions node.childStorage(i) were not implemented correctly
    in all node implementations. But they cannot be removed right now because
    they are used in the tree transformation methods.
-   The node.childStorage(i) now consistently take an integral-constant or a
    runtime index as first argument. This allows for more generic
    implementations.

### To Do

-   [x] Check compatibility with dune-pdelab
-   [x] Check compatibility with dune-functions

See merge request [!72]

  [!72]: gitlab.dune-project.org/staging/dune-typetree/merge_requests/72

CHANGELOG.md

@@ -4,6 +4,12 @@ Changes
 TypeTree 2.8-git
 ----------------
 
+-   Removed the type aliases for the storage type `Storage` and `ConstStorage` in the
+    node implementations.
+-   The memberfunction `childStorage()` in the nodes now consistently take an index or an
+    index_constant as argument.
+
+
 TypeTree 2.7
 ------------
 

dune/typetree/childextraction.hh

@@ -4,6 +4,7 @@
 #ifndef DUNE_TYPETREE_CHILDEXTRACTION_HH
 #define DUNE_TYPETREE_CHILDEXTRACTION_HH
 
+#include <type_traits>
 #include <utility>
 
 #include <dune/common/concept.hh>
@@ -25,129 +26,75 @@ namespace Dune {
 
 #ifndef DOXYGEN
 
-    namespace impl {
-
-      // ********************************************************************************
-      // end of the recursion, there are no child indices, so just return the node itself
-      // ********************************************************************************
-
-      struct IsPointerLike {
-        template <class Node>
-        auto require(const Node& node) -> decltype(*node);
-      };
+    namespace Impl {
 
-      template<typename Node>
-      auto child(Node&& node) -> decltype(std::forward<Node>(node))
+      // check at run time whether index is a valid child index
+      template <class Node, class Index>
+      std::true_type checkChildIndex (Node const& node, Index i)
       {
-        return std::forward<Node>(node);
+        assert(i < node.degree() && "Child index out of range");
+        return {};
       }
 
-      // for now, this wants the passed-in object to be pointer-like. I don't know how clever
-      // that is in the long run, though.
-      template<typename Node, typename std::enable_if_t<Dune::models<IsPointerLike,Node>(),int> = 0>
-      auto childStorage(Node&& node)
+      // check at compile time whether index is a valid index
+      template <class Node, std::size_t i>
+      std::bool_constant<(i < Node::degree())> checkChildIndex (Node const& node, index_constant<i>)
       {
-        return std::forward<Node>(node);
+        static_assert(i < Node::degree(), "Child index out of range");
+        return {};
       }
 
-      // ********************************************************************************
-      // next index is a compile-time constant
-      // ********************************************************************************
-
-      // we need a concept to make sure that the node has a templated child()
-      // method
-      struct HasTemplateChildMethod {
-        template <class Node>
-        auto require(const Node& node) -> decltype(node.template child<0>());
-      };
-
-      // The actual implementation is rather simple, we just use an overload that requires the first index
-      // to be an index_constant, get the child and then recurse.
-      // It only gets ugly due to the enable_if, but without that trick, the error messages for the user
-      // can get *very* obscure (they are bad enough as it is).
-      template<typename Node, std::size_t i, typename... J,
-        typename std::enable_if<
-          Dune::models<HasTemplateChildMethod, Node>() &&
-          (i < StaticDegree<Node>::value), int>::type = 0>
-      decltype(auto) child(Node&& node, index_constant<i>, J... j)
-      {
-        return child(std::forward<Node>(node).template child<i>(),j...);
-      }
-
-      template<typename Node, std::size_t i, typename... J,
-        typename std::enable_if<
-          Dune::models<HasTemplateChildMethod, decltype(*std::declval<std::decay_t<Node>>())>() &&
-        (i < StaticDegree<decltype(*std::declval<Node>())>::value), int>::type = 0>
-      decltype(auto) childStorage(Node&& node, index_constant<i>, J... j)
+      // finally return the node itself if no further indices are provided. Break condition
+      // for the recursion over the node childs.
+      template<class Node>
+      decltype(auto) childImpl (Node&& node)
       {
-        return childStorage(std::forward<Node>(node)->template childStorage<i>(),j...);
+        return std::forward<Node>(node);
       }
 
-      // This overload is only present to give useful compiler
-      // error messages via static_assert in case the other overloads
-      // fail.
-      template<typename Node, std::size_t i, typename... J,
-        typename std::enable_if<
-          (!Dune::models<HasTemplateChildMethod, Node>()) ||
-          (i >= StaticDegree<Node>::value), int>::type = 0>
-      void child(Node&& node, index_constant<i>, J... j)
+      template<class NodePtr>
+      auto childStorageImpl (NodePtr&& nodePtr)
       {
-        static_assert(Dune::models<HasTemplateChildMethod, Node>(), "Node does not have a template method child()");
-        static_assert(i < StaticDegree<Node>::value, "Child index out of range");
+        return std::forward<NodePtr>(nodePtr);
       }
 
-      // ********************************************************************************
-      // next index is a run-time value
-      // ********************************************************************************
-
-      // The actual implemention here overloads on std::size_t. It is a little less ugly because it currently
-      // has a hard requirement on the PowerNode Tag (although only using is_convertible, as tags can be
-      // inherited (important!).
-      template<typename Node, typename... J>
-      decltype(auto)
-      child(
-        Node&& node,
-        std::enable_if_t<
-          std::is_convertible<
-            NodeTag<Node>,
-            PowerNodeTag
-          >{},
-          std::size_t> i,
-        J... j
-        )
+      // recursively call `node.child(...)` with the given indices
+      template<class Node, class I0, class... I>
+      decltype(auto) childImpl (Node&& node, I0 i0, I... i)
       {
-        return child(std::forward<Node>(node).child(i),j...);
+        auto valid = checkChildIndex(node,i0);
+        if constexpr (valid)
+          return childImpl(node.child(i0),i...);
+        else
+          return;
       }
 
-      template<typename Node, typename... J>
-      decltype(auto)
-      childStorage(
-        Node&& node,
-        std::enable_if_t<
-          std::is_convertible<
-            NodeTag<decltype(*std::declval<Node>())>,
-            PowerNodeTag
-          >{},
-          std::size_t> i,
-        J... j
-        )
+      // recursively call `node.childStorage(...)` with the given indices
+      template<class NodePtr, class I0, class... I>
+      decltype(auto) childStorageImpl (NodePtr&& nodePtr, I0 i0, I... i)
       {
-        return childStorage(std::forward<Node>(node)->childStorage(i),j...);
+        auto valid = checkChildIndex(*nodePtr,i0);
+        if constexpr (valid)
+          return childStorageImpl(nodePtr->childStorage(i0),i...);
+        else
+          return;
       }
 
-      template<typename Node, typename... Indices, std::size_t... i>
-      decltype(auto) child(Node&& node, HybridTreePath<Indices...> tp, std::index_sequence<i...>)
+      // forward to the impl methods by extracting the indices from the treepath
+      template<class Node, class... Indices, std::size_t... i>
+      decltype(auto) child (Node&& node, HybridTreePath<Indices...> tp, std::index_sequence<i...>)
       {
-        return child(std::forward<Node>(node),treePathEntry<i>(tp)...);
+        return childImpl(std::forward<Node>(node),treePathEntry<i>(tp)...);
       }
 
-      template<typename Node, typename... Indices, std::size_t... i>
-      decltype(auto) childStorage(Node&& node, HybridTreePath<Indices...> tp, std::index_sequence<i...>)
+      // forward to the impl methods by extracting the indices from the treepath
+      template<class NodePtr, class... Indices, std::size_t... i>
+      decltype(auto) childStorage (NodePtr&& nodePtr, HybridTreePath<Indices...> tp, std::index_sequence<i...>)
       {
-        return childStorage(std::forward<Node>(node),treePathEntry<i>(tp)...);
+        return childStorageImpl(std::forward<NodePtr>(nodePtr),treePathEntry<i>(tp)...);
       }
 
-    } // namespace imp
+    } // end namespace Impl
 
 #endif // DOXYGEN
 
@@ -176,23 +123,23 @@ namespace Dune {
      */
     template<typename Node, typename... Indices>
 #ifdef DOXYGEN
-    ImplementationDefined child(Node&& node, Indices... indices)
+    ImplementationDefined child (Node&& node, Indices... indices)
 #else
-    decltype(auto) child(Node&& node, Indices... indices)
+    decltype(auto) child (Node&& node, Indices... indices)
 #endif
     {
-      return impl::child(std::forward<Node>(node),indices...);
+      return Impl::childImpl(std::forward<Node>(node),indices...);
     }
 
     template<typename Node, typename... Indices>
 #ifdef DOXYGEN
-    ImplementationDefined childStorage(Node&& node, Indices... indices)
+    ImplementationDefined childStorage (Node&& node, Indices... indices)
 #else
-    auto childStorage(Node&& node, Indices... indices)
+    auto childStorage (Node&& node, Indices... indices)
 #endif
     {
-      //static_assert(sizeof...(Indices) > 0, "childStorage() cannot be called with an empty list of child indices");
-      return impl::childStorage(&node,indices...);
+      static_assert(sizeof...(Indices) > 0, "childStorage() cannot be called with an empty list of child indices");
+      return Impl::childStorageImpl(&node,indices...);
     }
 
     //! Extracts the child of a node given by a static TreePath object.
@@ -216,23 +163,23 @@ namespace Dune {
      */
     template<typename Node, std::size_t... Indices>
 #ifdef DOXYGEN
-    ImplementationDefined child(Node&& node, TreePath<Indices...> treePath)
+    ImplementationDefined child (Node&& node, TreePath<Indices...> treePath)
 #else
-    decltype(auto) child(Node&& node, TreePath<Indices...>)
+    decltype(auto) child (Node&& node, TreePath<Indices...>)
 #endif
     {
-      return child(std::forward<Node>(node),index_constant<Indices>{}...);
+      return Impl::childImpl(std::forward<Node>(node),index_constant<Indices>{}...);
     }
 
     template<typename Node, std::size_t... Indices>
 #ifdef DOXYGEN
-    ImplementationDefined childStorage(Node&& node, TreePath<Indices...> treePath)
+    ImplementationDefined childStorage (Node&& node, TreePath<Indices...> treePath)
 #else
-    auto childStorage(Node&& node, TreePath<Indices...>)
+    auto childStorage (Node&& node, TreePath<Indices...>)
 #endif
     {
       static_assert(sizeof...(Indices) > 0, "childStorage() cannot be called with an empty TreePath");
-      return impl::childStorage(&node,index_constant<Indices>{}...);
+      return Impl::childStorageImpl(&node,index_constant<Indices>{}...);
     }
 
     //! Extracts the child of a node given by a HybridTreePath object.
@@ -261,23 +208,23 @@ namespace Dune {
      */
     template<typename Node, typename... Indices>
 #ifdef DOXYGEN
-    ImplementationDefined child(Node&& node, HybridTreePath<Indices...> treePath)
+    ImplementationDefined child (Node&& node, HybridTreePath<Indices...> treePath)
 #else
-    decltype(auto) child(Node&& node, HybridTreePath<Indices...> tp)
+    decltype(auto) child (Node&& node, HybridTreePath<Indices...> tp)
 #endif
     {
-      return impl::child(std::forward<Node>(node),tp,std::index_sequence_for<Indices...>{});
+      return Impl::child(std::forward<Node>(node),tp,std::index_sequence_for<Indices...>{});
     }
 
     template<typename Node, typename... Indices>
 #ifdef DOXYGEN
-    ImplementationDefined child(Node&& node, HybridTreePath<Indices...> treePath)
+    ImplementationDefined child (Node&& node, HybridTreePath<Indices...> treePath)
 #else
-    auto childStorage(Node&& node, HybridTreePath<Indices...> tp)
+    auto childStorage (Node&& node, HybridTreePath<Indices...> tp)
 #endif
     {
       static_assert(sizeof...(Indices) > 0, "childStorage() cannot be called with an empty TreePath");
-      return impl::childStorage(&node,tp,std::index_sequence_for<Indices...>{});
+      return Impl::childStorage(&node,tp,std::index_sequence_for<Indices...>{});
     }
 
 
@@ -386,7 +333,7 @@ namespace Dune {
         Dune::TypeTree::is_flat_index<T>::value,
         bool
         >::type
-      _non_empty_tree_path(T)
+      _non_empty_tree_path (T)
       {
         return false;
       }
@@ -396,7 +343,7 @@ namespace Dune {
         !Dune::TypeTree::is_flat_index<T>::value,
         bool
         >::type
-      _non_empty_tree_path(T t)
+      _non_empty_tree_path (T t)
       {
         return treePathSize(t) > 0;
       }

dune/typetree/compositenode.hh

@@ -47,7 +47,7 @@ namespace Dune {
       //! The number of children.
       static const std::size_t CHILDREN = sizeof...(Children);
 
-      static constexpr std::size_t degree()
+      static constexpr std::size_t degree ()
       {
         return sizeof...(Children);
       }
@@ -63,82 +63,73 @@ namespace Dune {
 
         //! The type of the child.
         typedef typename std::tuple_element<k,ChildTypes>::type type;
-
-        //! The storage type of the child.
-        typedef typename std::tuple_element<k,NodeStorage>::type Storage;
-
-        //! The const storage type of the child.
-        typedef std::shared_ptr<const typename std::tuple_element<k,ChildTypes>::type> ConstStorage;
       };
 
       //! @name Child Access
       //! @{
 
-      //! Returns the i-th child.
+      //! Returns the k-th child.
       /**
-       * \returns a reference to the i-th child.
+       * \returns a reference to the k-th child.
        */
       template<std::size_t k>
-      typename Child<k>::Type& child(index_constant<k> = {})
+      typename Child<k>::Type& child (index_constant<k> = {})
       {
         return *std::get<k>(_children);
       }
 
-      //! Returns the i-th child (const version).
+      //! Returns the k-th child (const version).
       /**
-       * \returns a const reference to the i-th child.
+       * \returns a const reference to the k-th child.
        */
       template<std::size_t k>
-      const typename Child<k>::Type& child(index_constant<k> = {}) const
+      const typename Child<k>::Type& child (index_constant<k> = {}) const
       {
         return *std::get<k>(_children);
       }
 
-      //! Returns the storage of the i-th child.
+      //! Returns the storage of the k-th child.
       /**
-       * \returns a copy of the object storing the i-th child.
+       * \returns a copy of the object storing the k-th child.
        */
       template<std::size_t k>
-      typename Child<k>::Storage childStorage(index_constant<k> = {})
+      std::shared_ptr<typename Child<k>::Type> childStorage (index_constant<k> = {})
       {
         return std::get<k>(_children);
       }
 
-      //! Returns the storage of the i-th child (const version).
+      //! Returns the storage of the k-th child (const version).
       /**
-       * This method is only important if the child is stored as
-       * some kind of pointer, as this allows the pointee type to
-       * become const.
-       * \returns a copy of the object storing the i-th child.
+       * \returns a copy of the object storing the k-th child.
        */
       template<std::size_t k>
-      typename Child<k>::ConstStorage childStorage(index_constant<k> = {}) const
+      std::shared_ptr<const typename Child<k>::Type> childStorage (index_constant<k> = {}) const
       {
         return std::get<k>(_children);
       }
 
-      //! Sets the i-th child to the passed-in value.
+      //! Sets the k-th child to the passed-in value.
       template<std::size_t k>
-      void setChild(typename Child<k>::Type& child, index_constant<k> = {})
+      void setChild (typename Child<k>::Type& child, index_constant<k> = {})
       {
         std::get<k>(_children) = stackobject_to_shared_ptr(child);
       }
 
       //! Store the passed value in k-th child.
       template<std::size_t k>
-      void setChild(typename Child<k>::Type&& child, index_constant<k> = {})
+      void setChild (typename Child<k>::Type&& child, index_constant<k> = {})
       {
         std::get<k>(_children) = convert_arg(std::move(child));
       }
 
-      //! Sets the storage of the i-th child to the passed-in value.
+      //! Sets the storage of the k-th child to the passed-in value.
       template<std::size_t k>
-      void setChild(typename Child<k>::Storage child, index_constant<k> = {})
+      void setChild (std::shared_ptr<typename Child<k>::Type> child, index_constant<k> = {})
       {
-        std::get<k>(_children) = child;
+        std::get<k>(_children) = std::move(child);
       }
 
-      const NodeStorage& nodeStorage() const
+      const NodeStorage& nodeStorage () const
       {
         return _children;
       }
@@ -172,11 +163,11 @@ namespace Dune {
        */
 #ifdef DOXYGEN
       template<typename... Indices>
-      ImplementationDefined& child(Indices... indices)
+      ImplementationDefined& child (Indices... indices)
 #else
       template<typename I0, typename... I,
         std::enable_if_t<(sizeof...(I) > 0) || IsTreePath<I0>::value, int > = 0>
-      decltype(auto) child(I0 i0, I... i)
+      decltype(auto) child (I0 i0, I... i)
 #endif
       {
         static_assert(sizeof...(I) > 0 || impl::_non_empty_tree_path(I0{}),
@@ -192,11 +183,11 @@ namespace Dune {
        */
 #ifdef DOXYGEN
       template<typename... Indices>
-      const ImplementationDefined& child(Indices... indices)
+      const ImplementationDefined& child (Indices... indices)
 #else
       template<typename I0, typename... I,
         std::enable_if_t<(sizeof...(I) > 0) || IsTreePath<I0>::value, int > = 0>
-      decltype(auto) child(I0 i0, I... i) const
+      decltype(auto) child (I0 i0, I... i) const
 #endif
       {
         static_assert(sizeof...(I) > 0 || impl::_non_empty_tree_path(I0{}),
@@ -220,22 +211,22 @@ namespace Dune {
        * will not be usable before its children are set using any of the
        * setChild(...) methods!
        */
-      CompositeNode()
+      CompositeNode ()
       {}
 
       //! Initialize all children with the passed-in objects.
       template<typename... Args, typename = typename std::enable_if<(sizeof...(Args) == CHILDREN)>::type>
-      CompositeNode(Args&&... args)
+      CompositeNode (Args&&... args)
         : _children(convert_arg(std::forward<Args>(args))...)
       {}
 
       //! Initialize the CompositeNode with copies of the passed in Storage objects.
-      CompositeNode(std::shared_ptr<Children>... children)
-        : _children(children...)
+      CompositeNode (std::shared_ptr<Children>... children)
+        : _children(std::move(children)...)
       {}
 
       //! Initialize the CompositeNode with a copy of the passed-in storage type.
-      CompositeNode(const NodeStorage& children)
+      CompositeNode (const NodeStorage& children)
         : _children(children)
       {}
 

dune/typetree/filteredcompositenode.hh

@@ -115,7 +115,7 @@ namespace Dune {
       //! The number of children.
       static const std::size_t CHILDREN = filter_result::size;
 
-      static constexpr std::size_t degree()
+      static constexpr std::size_t degree ()
       {
         return filter_result::size;
       }
@@ -137,74 +137,58 @@ namespace Dune {
 
         //! The type of the child.
         typedef typename OriginalChild::type type;
-
-        //! The storage type of the child.
-        typedef typename OriginalChild::Storage Storage;
-
-        //! The const storage type of the child.
-        typedef typename OriginalChild::ConstStorage ConstStorage;
       };
 
       //! @name Child Access
       //! @{
 
-      //! Returns the i-th child.
+      //! Returns the k-th child.
       /**
-       * \returns a reference to the i-th child.
+       * \returns a reference to the k-th child.
        */
-      template<std::size_t k>
-      typename std::enable_if<lazy_enable<k>::value,typename Child<k>::Type&>::type
-      child(Dune::index_constant<k> = {})
+      template<std::size_t k,
+        typename std::enable_if<lazy_enable<k>::value, int>::type = 0>
+      auto& child (index_constant<k> = {})
       {
         return _node->template child<Child<k>::mapped_index>();
       }
 
-      //! Returns the i-th child (const version).
+      //! Returns the k-th child (const version).
       /**
-       * \returns a const reference to the i-th child.
+       * \returns a const reference to the k-th child.
        */
       template<std::size_t k>
-      const typename Child<k>::Type& child(Dune::index_constant<k> = {}) const
+      const auto& child (index_constant<k> = {}) const
       {
         return _node->template child<Child<k>::mapped_index>();
       }
 
-      //! Returns the storage of the i-th child.
+      //! Returns the storage of the k-th child.
       /**
-       * \returns a copy of the object storing the i-th child.
+       * \returns a copy of the object storing the k-th child.
        */
-      template<std::size_t k>
-      typename std::enable_if<lazy_enable<k>::value,typename Child<k>::Storage>::type
-      childStorage()
+      template<std::size_t k,
+        typename std::enable_if<lazy_enable<k>::value, int>::type = 0>
+      auto childStorage (index_constant<k> = {})
       {
         return _node->template childStorage<Child<k>::mapped_index>();
       }
 
-      //! Returns the storage of the i-th child (const version).
+      //! Returns the storage of the k-th child (const version).
       /**
-       * This method is only important if the child is stored as
-       * some kind of pointer, as this allows the pointee type to
-       * become const.
-       * \returns a copy of the object storing the i-th child.
+       * \returns a copy of the object storing the k-th child.
        */
       template<std::size_t k>
-      typename Child<k>::ConstStorage childStorage() const
+      auto childStorage (index_constant<k> = {}) const
       {
         return _node->template childStorage<Child<k>::mapped_index>();
       }
 
-      //! Sets the i-th child to the passed-in value.
-      template<std::size_t k>
-      void setChild(typename Child<k>::type& child, typename std::enable_if<lazy_enable<k>::value,void*>::type = 0)
-      {
-        _node->template childStorage<Child<k>::mapped_index>() = stackobject_to_shared_ptr(child);
-      }
-
-      //! Sets the storage of the i-th child to the passed-in value.
-      template<std::size_t k>
-      void setChild(typename Child<k>::storage_type child, typename std::enable_if<lazy_enable<k>::value,void*>::type = 0)
+      //! Sets the k-th child to the passed-in value.
+      template<std::size_t k, class ChildType>
+      void setChild (ChildType&& child, typename std::enable_if<lazy_enable<k>::value,void*>::type = 0)
       {
-        _node->template childStorage<Child<k>::mapped_index>() = child;
+        _node->template setChild<Child<k>::mapped_index>(std::forward<ChildType>(child));
       }
 
       //! @}
@@ -220,7 +204,7 @@ namespace Dune {
        */
       template<bool enabled = !nodeIsConst>
       typename std::enable_if<enabled,Node&>::type
-      unfiltered()
+      unfiltered ()
       {
         return *_node;
       }
@@ -229,7 +213,7 @@ namespace Dune {
       /**
        * \returns A const reference to the original, unfiltered node.
        */
-      const Node& unfiltered() const
+      const Node& unfiltered () const
       {
         return *_node;
       }
@@ -240,7 +224,7 @@ namespace Dune {
        */
       template<bool enabled = !nodeIsConst>
       typename std::enable_if<enabled,std::shared_ptr<Node> >::type
-      unfilteredStorage()
+      unfilteredStorage ()
       {