Return reference from methods that cannot return a nullpointer

Previously some methods returned a pointer but would never return nullptr. This
changes their return type to references instead.

Author: danpoe

src/util/as_const.h

@@ -16,6 +16,13 @@ const T &as_const(T &value)
   return static_cast<const T &>(value);
 }
 
+/// Return a pointer to the same object but ensures the type is pointer to const
+template <typename T>
+const T *as_const_ptr(T *t)
+{
+  return t;
+}
+
 /// Deleted to avoid calling as_const on an xvalue
 template <typename T>
 void as_const(T &&) = delete;

src/util/sharing_map.h

@@ -27,6 +27,7 @@ Author: Daniel Poetzl
 #include <type_traits>
 #include <vector>
 
+#include "as_const.h"
 #include "irep.h"
 #include "optional.h"
 #include "sharing_node.h"
@@ -518,7 +519,7 @@ class sharing_mapt
 protected:
   // helpers
 
-  leaft *get_leaf_node(const key_type &k);
+  leaft &get_leaf_node(const key_type &k);
   const leaft *get_leaf_node(const key_type &k) const;
 
   /// Move a container node (containing a single leaf) further down the tree
@@ -1062,7 +1063,7 @@ ::get_delta_view(
   while(!stack.empty());
 }
 
-SHARING_MAPT2(, leaft *)::get_leaf_node(const key_type &k)
+SHARING_MAPT2(, leaft &)::get_leaf_node(const key_type &k)
 {
   SM_ASSERT(has_key(k));
 
@@ -1074,9 +1075,8 @@ SHARING_MAPT2(, leaft *)::get_leaf_node(const key_type &k)
   {
     std::size_t bit = key & mask;
 
-    ip = ip->add_child(bit);
-    SM_ASSERT(ip != nullptr);
-    SM_ASSERT(!ip->empty()); // since the key must exist in the map
+    ip = &ip->add_child(bit);
+    PRECONDITION(!ip->empty()); // since the key must exist in the map
 
     if(ip->is_internal())
     {
@@ -1086,17 +1086,14 @@ SHARING_MAPT2(, leaft *)::get_leaf_node(const key_type &k)
 
     if(ip->is_leaf())
     {
-      return ip;
+      return *ip;
     }
 
     if(ip->is_container())
     {
-      return ip->find_leaf(k);
+      return *ip->find_leaf(k);
     }
   }
-
-  UNREACHABLE;
-  return nullptr;
 }
 
 SHARING_MAPT2(const, leaft *)::get_leaf_node(const key_type &k) const
@@ -1154,15 +1151,15 @@ SHARING_MAPT(void)::erase(const key_type &k)
   {
     std::size_t bit = key & mask;
 
-    const to_mapt &m = as_const(ip)->get_to_map();
+    const to_mapt &m = as_const_ptr(ip)->get_to_map();
 
     if(m.size() > 1 || del == nullptr)
     {
       del = ip;
       del_bit=bit;
     }
 
-    ip = ip->add_child(bit);
+    ip = &ip->add_child(bit);
 
     PRECONDITION(!ip->empty());
 
@@ -1185,7 +1182,7 @@ SHARING_MAPT(void)::erase(const key_type &k)
     if(ip->is_container())
     {
       PRECONDITION(!ip->empty());
-      const leaf_listt &ll = as_const(ip)->get_container();
+      const leaf_listt &ll = as_const_ptr(ip)->get_container();
       PRECONDITION(!ll.empty());
 
       // forward list has one element
@@ -1220,7 +1217,7 @@ ::migrate(
   SM_ASSERT(starting_level < levels - 1);
   SM_ASSERT(inner.is_defined_internal());
 
-  leaft &leaf = *inner.add_child(bit_last);
+  leaft &leaf = inner.add_child(bit_last);
   SM_ASSERT(leaf.is_defined_leaf());
 
   std::size_t key_existing = hash()(leaf.get_key());
@@ -1253,19 +1250,19 @@ ::migrate(
     {
       // Place found
 
-      innert *l1 = ip->add_child(bit_existing);
-      SM_ASSERT(l1->empty());
-      l1->swap(leaf_kept);
+      innert &l1 = ip->add_child(bit_existing);
+      SM_ASSERT(l1.empty());
+      l1.swap(leaf_kept);
 
-      innert *l2 = ip->add_child(bit);
-      SM_ASSERT(l2->empty());
-      l2->make_leaf(k, std::forward<valueU>(m));
+      innert &l2 = ip->add_child(bit);
+      SM_ASSERT(l2.empty());
+      l2.make_leaf(k, std::forward<valueU>(m));
 
       return;
     }
 
     SM_ASSERT(bit == bit_existing);
-    ip = ip->add_child(bit);
+    ip = &ip->add_child(bit);
 
     key >>= chunk;
     key_existing >>= chunk;
@@ -1307,41 +1304,41 @@ ::insert(const key_type &k, valueU &&m)
     SM_ASSERT(ip->is_internal());
     SM_ASSERT(level == 0 || !ip->empty());
 
-    innert *child = ip->add_child(bit);
+    innert &child = ip->add_child(bit);
 
     // Place is unoccupied
-    if(child->empty())
+    if(child.empty())
     {
       if(level < levels - 1)
       {
         // Create leaf
-        child->make_leaf(k, m);
+        child.make_leaf(k, m);
       }
       else
       {
         SM_ASSERT(level == levels - 1);
 
         // Create container and insert leaf
-        child->place_leaf(k, std::forward<valueU>(m));
+        child.place_leaf(k, std::forward<valueU>(m));
 
-        SM_ASSERT(child->is_defined_container());
+        SM_ASSERT(child.is_defined_container());
       }
 
       num++;
 
       return;
     }
 
-    if(child->is_internal())
+    if(child.is_internal())
     {
-      ip = child;
+      ip = &child;
       key >>= chunk;
       level++;
 
       continue;
     }
 
-    if(child->is_leaf())
+    if(child.is_leaf())
     {
       // migrate leaf downwards
       migrate(level, key, bit, *ip, k, std::forward<valueU>(m));
@@ -1351,12 +1348,12 @@ ::insert(const key_type &k, valueU &&m)
       return;
     }
 
-    if(child->is_container())
+    if(child.is_container())
     {
       SM_ASSERT(level == levels - 1);
 
       // Add to the container
-      child->place_leaf(k, std::forward<valueU>(m));
+      child.place_leaf(k, std::forward<valueU>(m));
 
       num++;
 
@@ -1368,28 +1365,26 @@ ::insert(const key_type &k, valueU &&m)
 SHARING_MAPT4(valueU, void)
 ::replace(const key_type &k, valueU &&m)
 {
-  leaft *lp = get_leaf_node(k);
-  PRECONDITION(lp != nullptr); // key must exist in map
+  leaft &lp = get_leaf_node(k);
 
   INVARIANT(
-    !value_equalt()(as_const(lp)->get_value(), m),
+    !value_equalt()(as_const(lp).get_value(), m),
     "values should not be replaced with equal values to maximize sharing");
 
-  lp->set_value(std::forward<valueU>(m));
+  lp.set_value(std::forward<valueU>(m));
 }
 
 SHARING_MAPT(void)
 ::update(const key_type &k, std::function<void(mapped_type &)> mutator)
 {
-  leaft *lp = get_leaf_node(k);
-  PRECONDITION(lp != nullptr); // key must exist in map
+  leaft &lp = get_leaf_node(k);
 
-  value_comparatort comparator(as_const(lp)->get_value());
+  value_comparatort comparator(as_const(lp).get_value());
 
-  lp->mutate_value(mutator);
+  lp.mutate_value(mutator);
 
   INVARIANT(
-    !comparator(as_const(lp)->get_value()),
+    !comparator(as_const(lp).get_value()),
     "sharing_mapt::update should make some change. Consider using read-only "
     "method to check if an update is needed beforehand");
 }

src/util/sharing_node.h

@@ -33,6 +33,7 @@ Author: Daniel Poetzl
 #include <map>
 #endif
 
+#include "as_const.h"
 #include "invariant.h"
 #include "make_unique.h"
 #include "small_shared_n_way_ptr.h"
@@ -61,12 +62,6 @@ Author: Daniel Poetzl
 #define SN_PTR_TYPE_ARGS \
   d_internalt<SN_TYPE_ARGS>, d_containert<SN_TYPE_ARGS>, d_leaft<SN_TYPE_ARGS>
 
-template <class T>
-const T *as_const(T *t)
-{
-  return t;
-}
-
 // Inner nodes (internal nodes or container nodes)
 
 typedef small_shared_n_way_pointee_baset<3, unsigned> d_baset;
@@ -291,19 +286,17 @@ SN_TYPE_PAR_DEF class sharing_nodet
 
   // add leaf, key must not exist yet
   template <class valueU>
-  leaft *place_leaf(const keyT &k, valueU &&v)
+  void place_leaf(const keyT &k, valueU &&v)
   {
     SN_ASSERT(is_container()); // empty() is allowed
 
     // we need to check empty() first as the const version of find_leaf() must
     // not be called on an empty node
-    PRECONDITION(empty() || as_const(this)->find_leaf(k) == nullptr);
+    PRECONDITION(empty() || as_const(*this).find_leaf(k) == nullptr);
 
     leaf_listt &c = get_container();
     c.emplace_front(k, std::forward<valueU>(v));
     SN_ASSERT(c.front().is_defined_leaf());
-
-    return &c.front();
   }
 
   // remove leaf, key must exist
@@ -361,12 +354,12 @@ SN_TYPE_PAR_DEF class sharing_nodet
     return nullptr;
   }
 
-  typename d_it::innert *add_child(const std::size_t n)
+  typename d_it::innert &add_child(const std::size_t n)
   {
     SN_ASSERT(is_internal()); // empty() is allowed
 
     to_mapt &m = get_to_map();
-    return &m[n];
+    return m[n];
   }
 
   void remove_child(const std::size_t n)

unit/util/sharing_node.cpp

@@ -178,71 +178,6 @@ TEST_CASE("Sharing node", "[core][util]")
 
       REQUIRE(i.get_to_map().empty());
       REQUIRE(ci.find_child(0) == nullptr);
-
-      innert *p;
-
-      p = i.add_child(1);
-
-      REQUIRE(p != nullptr);
     }
   }
-
-  SECTION("Combined")
-  {
-    typedef sharing_nodet<int, int> leaft;
-    typedef sharing_nodet<int, int> innert;
-
-    innert map;
-
-    REQUIRE(map.empty());
-
-    innert *ip;
-    innert *cp;
-    leaft *lp;
-
-    // Add 0 -> 0 -> (0, 1)
-
-    ip = &map;
-
-    ip = ip->add_child(0);
-    REQUIRE(ip != nullptr);
-
-    cp = ip->add_child(0);
-    REQUIRE(cp != nullptr);
-
-    lp = cp->place_leaf(0, 1);
-    REQUIRE(lp != nullptr);
-
-    // Add 1 -> 2 -> (3, 4)
-
-    ip = &map;
-
-    ip = ip->add_child(1);
-    REQUIRE(ip != nullptr);
-
-    cp = ip->add_child(2);
-    REQUIRE(cp != nullptr);
-
-    lp = cp->place_leaf(3, 4);
-    REQUIRE(lp != nullptr);
-
-    // Add 1 -> 3 -> (4, 5)
-
-    ip = &map;
-
-    ip = ip->add_child(1);
-    REQUIRE(ip != nullptr);
-
-    cp = ip->add_child(3);
-    REQUIRE(cp != nullptr);
-
-    lp = cp->place_leaf(4, 5);
-    REQUIRE(lp != nullptr);
-
-    // Copy
-
-    innert map2(map);
-
-    REQUIRE(map2.shares_with(map));
-  }
 }