storage/model/typed_value - fixes after code review

include/storage/model/typed_value.hpp

@@ -10,8 +10,6 @@
 #include "utils/total_ordering.hpp"
 #include "utils/exceptions/basic_exception.hpp"
 
-using std::string;
-
 /**
  * Encapsulation of a value and it's type encapsulated in a class that has no
  * compiled-time info about that type.
@@ -45,11 +43,11 @@ public:
   TypedValue(float value) : type_(Type::Float) { float_v = value; }
 
   /// constructors for non-primitive types (shared pointers)
-  TypedValue(const string &value) : type_(Type::String) {
+  TypedValue(const std::string &value) : type_(Type::String) {
-    new (&string_v) std::shared_ptr<string>(new string(value));
+    new (&string_v) std::shared_ptr<std::string>(new std::string(value));
   }
   TypedValue(const char* value) : type_(Type::String) {
-    new (&string_v) std::shared_ptr<string>(new string(value));
+    new (&string_v) std::shared_ptr<std::string>(new std::string(value));
   }
 
   // assignment ops

include/storage/model/typed_value_store.hpp

@@ -41,17 +41,7 @@ public:
    * @param value  The value to set.
    */
   template<typename TValue>
-  void set(const TKey &key, const TValue &value) {
+  void set(const TKey &key, const TValue &value);
-    for (auto& kv: props_)
-      if (kv.first == key) {
-        kv.second = TypedValue(value);
-        return;
-      }
-
-    // there is no value for the given key, add new
-    // TODO consider vector size increment optimization
-    props_.push_back(std::move(std::make_pair(key, value)));
-  }
 
   /**
    * Set overriding for character constants. Forces conversion
@@ -62,9 +52,7 @@ public:
    * value at the same key (if there was one) is replaced.
    * @param value  The value to set.
    */
-  void set(const TKey &key, const char *value) {
+  void set(const TKey &key, const char *value);
-    set(key, std::string(value));
-  }
 
   /**
    * Removes the TypedValue for the given key.

include/utils/assert.hpp

@@ -41,6 +41,11 @@
         std::exit(EXIT_FAILURE);                                               \
     }
 
+#define permanent_fail(message)                                                \
+        std::ostringstream s;                                                  \
+        s << message;                                                          \
+        __handle_assert_message(s.str());                                      \
+        std::exit(EXIT_FAILURE);                                               \
 /**
  * runtime assertion is more like standart C assert but with custom
  * define which controls when the assertion will be active

src/storage/model/typed_value.cpp

@@ -4,54 +4,56 @@
 #include <fmt/format.h>
 
 #include "storage/model/typed_value.hpp"
+#include "utils/assert.hpp"
 
 // Value extraction template instantiations
 template<>
 bool TypedValue::Value<bool>() const {
-  assert(type_ == TypedValue::Type::Bool);
+  runtime_assert(type_ == TypedValue::Type::Bool, "Incompatible template param and type");
   return bool_v;
 }
 
 template<>
-string TypedValue::Value<string>() const {
+std::string TypedValue::Value<std::string>() const {
-  assert(type_ == TypedValue::Type::String);
+  runtime_assert(type_ == TypedValue::Type::String, "Incompatible template param and type");
   return *string_v;
 }
 
 template<>
 int TypedValue::Value<int>() const {
-  assert(type_ == TypedValue::Type::Int);
+  runtime_assert(type_ == TypedValue::Type::Int, "Incompatible template param and type");
   return int_v;
 }
 
 template<>
 float TypedValue::Value<float>() const {
-  assert(type_ == TypedValue::Type::Float);
+  runtime_assert(type_ == TypedValue::Type::Float, "Incompatible template param and type");
   return float_v;
 }
 
 TypedValue::TypedValue(const TypedValue &other) : type_(other.type_) {
   switch (other.type_) {
-
     case TypedValue::Type::Null:
-      break;
+      return;
 
     case TypedValue::Type::Bool:
       this->bool_v = other.bool_v;
-      break;
+      return;
 
     case TypedValue::Type::String:
-      new(&string_v) std::shared_ptr<string>(other.string_v);
+      new(&string_v) std::shared_ptr<std::string>(other.string_v);
-      break;
+      return;
 
     case Type::Int:
       this->int_v = other.int_v;
-      break;
+      return;
 
     case Type::Float:
       this->float_v = other.float_v;
-      break;
+      return;
   }
+
+  permanent_fail("Unsupported TypedValue::Type");
 }
 
 std::ostream &operator<<(std::ostream &os, const TypedValue::Type type) {
@@ -67,6 +69,7 @@ std::ostream &operator<<(std::ostream &os, const TypedValue::Type type) {
     case TypedValue::Type::Float:
       return os << "float";
   }
+  permanent_fail("Unsupported TypedValue::Type");
 }
 
 std::ostream &operator<<(std::ostream &os, const TypedValue &property) {
@@ -82,6 +85,7 @@ std::ostream &operator<<(std::ostream &os, const TypedValue &property) {
     case TypedValue::Type::Float:
       return os << property.Value<float>();
   }
+  permanent_fail("Unsupported TypedValue::Type");
 }
 
 TypedValue &TypedValue::operator=(TypedValue &&other) {
@@ -94,21 +98,20 @@ TypedValue &TypedValue::operator=(TypedValue &&other) {
       case TypedValue::Type::Null:
       case TypedValue::Type::Bool:
         this->bool_v = other.bool_v;
-        break;
+        return *this;
       case TypedValue::Type::String:
         this->string_v = std::move(other.string_v);
-        break;
+        return *this;
       case TypedValue::Type::Int:
         this->int_v = other.int_v;
-        break;
+        return *this;
       case TypedValue::Type::Float:
         this->float_v = other.float_v;
-        break;
-    }
-  }
-
         return *this;
     }
+  }
+  permanent_fail("Unsupported TypedValue::Type");
+}
 
 const TypedValue TypedValue::Null = TypedValue();
 
@@ -117,19 +120,17 @@ TypedValue::~TypedValue() {
   switch (type_) {
     // destructor for primitive types does nothing
     case Type::Null:
-      break;
     case Type::Bool:
-      break;
     case Type::Int:
-      break;
     case Type::Float:
-      break;
+      return;
 
       // destructor for shared pointer must release
     case Type::String:
-      string_v.~shared_ptr<string>();
+      string_v.~shared_ptr<std::string>();
-      break;
+      return;
   }
+  permanent_fail("Unsupported TypedValue::Type");
 }
 
 /**
@@ -145,11 +146,9 @@ float ToFloat(const TypedValue& prop) {
       return (float)prop.Value<int>();
     case TypedValue::Type::Float:
       return prop.Value<float>();
-    case TypedValue::Type::Null:
+
-    case TypedValue::Type::String:
+    default:
-    case TypedValue::Type::Bool:
+      permanent_fail("Unsupported TypedValue::Type");
-      // TODO switch to production-exception
-      assert(false);
   }
 }
 
@@ -164,7 +163,7 @@ TypedValue operator<(const TypedValue& a, const TypedValue& b) {
     if (a.type_ != b.type_)
       throw TypedValueException("Invalid equality operand types({} + {})", a.type_, b.type_);
     else
-      return a.Value<string>() < b.Value<string>();
+      return a.Value<std::string>() < b.Value<std::string>();
   }
 
   // at this point we only have int and float
@@ -183,7 +182,7 @@ TypedValue operator==(const TypedValue& a, const TypedValue& b) {
     if (a.type_ != b.type_)
       throw TypedValueException("Invalid equality operand types({} + {})", a.type_, b.type_);
     else
-      return a.Value<string>() == b.Value<string>();
+      return a.Value<std::string>() == b.Value<std::string>();
   }
 
   if (a.type_ == TypedValue::Type::Bool || b.type_ == TypedValue::Type::Bool) {
@@ -206,31 +205,12 @@ TypedValue operator!(const TypedValue& a) {
       return TypedValue::Null;
     case TypedValue::Type::Bool:
       return TypedValue(!a.Value<bool>());
-    case TypedValue::Type::Int:
+
-    case TypedValue::Type::Float:
+    default:
-    case TypedValue::Type::String:
       throw TypedValueException("Invalid logical not operand type (!{})", a.type_);
   }
 }
 
-
-/*
- * Derived comparsion operators.
- */
-//TypedValue operator!=(const TypedValue& a, const TypedValue& b) {
-//  return !(a == b);
-//}
-//
-//TypedValue operator>(const TypedValue& a, const TypedValue& b) {
-//  return (a < b) && (a != b);
-//}
-//TypedValue operator>=(const TypedValue& a, const TypedValue& b) {
-//  return operator!(a < b);
-//}
-//TypedValue operator<=(const TypedValue& a, const TypedValue& b){
-//  return (a < b) || (a == b);
-//}
-
 /**
  * Turns a numeric or string property into a string.
  *
@@ -240,15 +220,15 @@ TypedValue operator!(const TypedValue& a) {
 std::string PropToString(const TypedValue& prop) {
   switch (prop.type_) {
     case TypedValue::Type::String:
-      return prop.Value<string>();
+      return prop.Value<std::string>();
     case TypedValue::Type::Int:
       return std::to_string(prop.Value<int>());
     case TypedValue::Type::Float:
       return fmt::format("{}", prop.Value<float>());
+
+    // unsupported situations
     default:
-      // TODO change to release-assert
+      permanent_fail("Unsupported TypedValue::Type");
-      // This should never happen
-      assert(false);
   }
 }
 
@@ -260,8 +240,8 @@ TypedValue operator-(const TypedValue &a) {
       return -a.Value<int>();
     case TypedValue::Type::Float:
       return -a.Value<float>();
-    case TypedValue::Type::Bool:
+
-    case TypedValue::Type::String:
+    default:
       throw TypedValueException("Invalid unary minus operand type (-{})", a.type_);
   }
 }
@@ -277,7 +257,8 @@ TypedValue operator-(const TypedValue &a) {
  *  @param op_name Name of the operation, used only for exception description,
  *  if raised.
  */
-inline void EnsureArithmeticallyOk(const TypedValue& a, const TypedValue& b, bool string_ok, const string& op_name) {
+inline void EnsureArithmeticallyOk(const TypedValue& a, const TypedValue& b,
+                                   bool string_ok, const std::string& op_name) {
   if (a.type_ == TypedValue::Type::Bool || b.type_ == TypedValue::Type::Bool)
     throw TypedValueException("Invalid {} operand types {}, {}", op_name, a.type_, b.type_);
 

src/storage/model/typed_value_store.cpp

@@ -10,6 +10,39 @@ const TypedValue& TypedValueStore::at(const TKey &key) const {
   return TypedValue::Null;
 }
 
+template<typename TValue>
+void TypedValueStore::set(const TKey &key, const TValue &value) {
+  for (auto& kv: props_)
+    if (kv.first == key) {
+      kv.second = TypedValue(value);
+      return;
+    }
+
+  // there is no value for the given key, add new
+  // TODO consider vector size increment optimization
+  props_.push_back(std::move(std::make_pair(key, value)));
+}
+
+// instantiations of the TypedValueStore::set function
+// instances must be made for all of the supported C++ types
+template void TypedValueStore::set<std::string>(const TKey &key, const std::string &value);
+template void TypedValueStore::set<bool>(const TKey &key, const bool &value);
+template void TypedValueStore::set<int>(const TKey &key, const int &value);
+template void TypedValueStore::set<float>(const TKey &key, const float &value);
+
+/**
+ * Set overriding for character constants. Forces conversion
+ * to std::string, otherwise templating might cast the pointer
+ * to something else (bool) and mess things up.
+ *
+ * @param key  The key for which the property is set. The previous
+ * value at the same key (if there was one) is replaced.
+ * @param value  The value to set.
+ */
+void TypedValueStore::set(const TKey &key, const char *value) {
+  set(key, std::string(value));
+}
+
 size_t TypedValueStore::erase(const TKey &key) {
   auto found = std::find_if(props_.begin(), props_.end(), [&key](std::pair<TKey, TypedValue> &kv){return kv.first == key;});
   if (found != props_.end()) {
@@ -33,3 +66,5 @@ void TypedValueStore::Accept(std::function<void(const TypedValueStore::TKey, con
     finish();
 
 }
+
+

tests/unit/typed_value.cpp

@@ -40,6 +40,7 @@ void EXPECT_PROP_NE(const TypedValue& a, const TypedValue& b)  {
 }
 
 TEST(TypedValue, CreationTypes) {
+  EXPECT_TRUE(TypedValue::Null.type_ == TypedValue::Type::Null);
 
   EXPECT_TRUE(TypedValue(true).type_ == TypedValue::Type::Bool);
   EXPECT_TRUE(TypedValue(false).type_ == TypedValue::Type::Bool);
@@ -58,8 +59,8 @@ TEST(TypedValue, CreationValues) {
   EXPECT_EQ(TypedValue(true).Value<bool>(), true);
   EXPECT_EQ(TypedValue(false).Value<bool>(), false);
 
-  EXPECT_EQ(TypedValue(std::string("bla")).Value<string>(), "bla");
+  EXPECT_EQ(TypedValue(std::string("bla")).Value<std::string>(), "bla");
-  EXPECT_EQ(TypedValue("bla2").Value<string>(), "bla2");
+  EXPECT_EQ(TypedValue("bla2").Value<std::string>(), "bla2");
 
   EXPECT_EQ(TypedValue(55).Value<int>(), 55);
 
@@ -90,7 +91,9 @@ TEST(TypedValue, Less) {
   for (int i = 0; i < props.size() + 1; ++i) {
     if (props.at(i).type_ == TypedValue::Type::Bool)
       continue;
-    EXPECT_THROW(props.at(i) < TypedValue(true), TypedValueException);
+    // the comparison should raise an exception
+    // cast to (void) so the compiler does not complain about unused comparison result
+    EXPECT_THROW((void)(props.at(i) < TypedValue(true)), TypedValueException);
   }
 
   // not_bool_type < Null = Null
@@ -183,13 +186,13 @@ TEST(TypedValue, Sum) {
   // sum of props of the same type
   EXPECT_EQ((TypedValue(2) + TypedValue(3)).Value<int>(), 5);
   EXPECT_FLOAT_EQ((TypedValue(2.5f) + TypedValue(1.25f)).Value<float>(), 3.75);
-  EXPECT_EQ((TypedValue("one") + TypedValue("two")).Value<string>(), "onetwo");
+  EXPECT_EQ((TypedValue("one") + TypedValue("two")).Value<std::string>(), "onetwo");
 
   // sum of string and numbers
-  EXPECT_EQ((TypedValue("one") + TypedValue(1)).Value<string>(), "one1");
+  EXPECT_EQ((TypedValue("one") + TypedValue(1)).Value<std::string>(), "one1");
-  EXPECT_EQ((TypedValue(1) + TypedValue("one")).Value<string>(), "1one");
+  EXPECT_EQ((TypedValue(1) + TypedValue("one")).Value<std::string>(), "1one");
-  EXPECT_EQ((TypedValue("one") + TypedValue(3.2f)).Value<string>(), "one3.2");
+  EXPECT_EQ((TypedValue("one") + TypedValue(3.2f)).Value<std::string>(), "one3.2");
-  EXPECT_EQ((TypedValue(3.2f) + TypedValue("one")).Value<string>(), "3.2one");
+  EXPECT_EQ((TypedValue(3.2f) + TypedValue("one")).Value<std::string>(), "3.2one");
 }
 
 TEST(TypedValue, Difference) {
@@ -250,13 +253,21 @@ TEST(TypedValue, TypeIncompatibility) {
       EXPECT_EQ(props.at(i).type_== props.at(j).type_, i == j);
 }
 
+/**
+ * Logical operations (logical and, or) are only legal on bools
+ * and nulls. This function ensures that the given
+ * logical operation throws exceptions when either operand
+ * is not bool or null.
+ *
+ * @param op The logical operation to test.
+ */
 void TestLogicalThrows(std::function<TypedValue(const TypedValue&, const TypedValue&)> op) {
   TypedValueStore props = MakePropsAllTypes();
   for (int i = 0; i < props.size() + 1; ++i) {
     auto p1 = props.at(i);
     for (int j = 0; j < props.size() + 1; ++j) {
       auto p2 = props.at(j);
-      // skip situations when p1 and p2 are either bool or null
+      // skip situations when both p1 and p2 are either bool or null
       auto p1_ok = p1.type_ == TypedValue::Type::Bool || p1.type_ == TypedValue::Type::Null;
       auto p2_ok = p2.type_ == TypedValue::Type::Bool || p2.type_ == TypedValue::Type::Null;
       if (p1_ok && p2_ok)
@@ -268,7 +279,7 @@ void TestLogicalThrows(std::function<TypedValue(const TypedValue&, const TypedVa
 }
 
 TEST(TypedValue, LogicalAnd) {
-//  TestLogicalThrows([](const TypedValue& p1, const TypedValue& p2) {return p1 && p2;});
+  TestLogicalThrows([](const TypedValue& p1, const TypedValue& p2) {return p1 && p2;});
   EXPECT_PROP_ISNULL(TypedValue::Null && TypedValue(true));
   EXPECT_PROP_EQ(TypedValue(true) && TypedValue(true), TypedValue(true));
   EXPECT_PROP_EQ(TypedValue(false) && TypedValue(true), TypedValue(false));