From 261797ea9ca5327c36cf11e1872a69f488c819e3 Mon Sep 17 00:00:00 2001 From: Florijan Stamenkovic Date: Thu, 2 Feb 2017 10:29:14 +0100 Subject: [PATCH] storage/model/typed_value - fixes after code review --- include/storage/model/typed_value.hpp | 8 +- include/storage/model/typed_value_store.hpp | 16 +--- include/utils/assert.hpp | 5 ++ src/storage/model/typed_value.cpp | 99 +++++++++------------ src/storage/model/typed_value_store.cpp | 35 ++++++++ tests/unit/typed_value.cpp | 31 ++++--- 6 files changed, 106 insertions(+), 88 deletions(-) diff --git a/include/storage/model/typed_value.hpp b/include/storage/model/typed_value.hpp index 01f27363b..4a0aaf296 100644 --- a/include/storage/model/typed_value.hpp +++ b/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) { - new (&string_v) std::shared_ptr(new string(value)); + TypedValue(const std::string &value) : type_(Type::String) { + new (&string_v) std::shared_ptr(new std::string(value)); } TypedValue(const char* value) : type_(Type::String) { - new (&string_v) std::shared_ptr(new string(value)); + new (&string_v) std::shared_ptr(new std::string(value)); } // assignment ops diff --git a/include/storage/model/typed_value_store.hpp b/include/storage/model/typed_value_store.hpp index d92fb6824..47c4be9c3 100644 --- a/include/storage/model/typed_value_store.hpp +++ b/include/storage/model/typed_value_store.hpp @@ -41,17 +41,7 @@ public: * @param value The value to set. */ template - 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))); - } + void set(const TKey &key, const TValue &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) { - set(key, std::string(value)); - } + void set(const TKey &key, const char *value); /** * Removes the TypedValue for the given key. diff --git a/include/utils/assert.hpp b/include/utils/assert.hpp index ac87544ad..842f183d2 100644 --- a/include/utils/assert.hpp +++ b/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 diff --git a/src/storage/model/typed_value.cpp b/src/storage/model/typed_value.cpp index 0211a6136..eeb4a39fe 100644 --- a/src/storage/model/typed_value.cpp +++ b/src/storage/model/typed_value.cpp @@ -4,54 +4,56 @@ #include #include "storage/model/typed_value.hpp" +#include "utils/assert.hpp" // Value extraction template instantiations template<> bool TypedValue::Value() const { - assert(type_ == TypedValue::Type::Bool); + runtime_assert(type_ == TypedValue::Type::Bool, "Incompatible template param and type"); return bool_v; } template<> -string TypedValue::Value() const { - assert(type_ == TypedValue::Type::String); +std::string TypedValue::Value() const { + runtime_assert(type_ == TypedValue::Type::String, "Incompatible template param and type"); return *string_v; } template<> int TypedValue::Value() const { - assert(type_ == TypedValue::Type::Int); + runtime_assert(type_ == TypedValue::Type::Int, "Incompatible template param and type"); return int_v; } template<> float TypedValue::Value() 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(other.string_v); - break; + new(&string_v) std::shared_ptr(other.string_v); + 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(); } + permanent_fail("Unsupported TypedValue::Type"); } TypedValue &TypedValue::operator=(TypedValue &&other) { @@ -94,20 +98,19 @@ 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; } } - - 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(); - break; + string_v.~shared_ptr(); + return; } + permanent_fail("Unsupported TypedValue::Type"); } /** @@ -145,11 +146,9 @@ float ToFloat(const TypedValue& prop) { return (float)prop.Value(); case TypedValue::Type::Float: return prop.Value(); - case TypedValue::Type::Null: - case TypedValue::Type::String: - case TypedValue::Type::Bool: - // TODO switch to production-exception - assert(false); + + default: + permanent_fail("Unsupported TypedValue::Type"); } } @@ -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() < b.Value(); + return a.Value() < b.Value(); } // 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() == b.Value(); + return a.Value() == b.Value(); } 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()); - case TypedValue::Type::Int: - case TypedValue::Type::Float: - case TypedValue::Type::String: + + default: 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(); + return prop.Value(); case TypedValue::Type::Int: return std::to_string(prop.Value()); case TypedValue::Type::Float: return fmt::format("{}", prop.Value()); + + // unsupported situations default: - // TODO change to release-assert - // This should never happen - assert(false); + permanent_fail("Unsupported TypedValue::Type"); } } @@ -260,8 +240,8 @@ TypedValue operator-(const TypedValue &a) { return -a.Value(); case TypedValue::Type::Float: return -a.Value(); - 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_); diff --git a/src/storage/model/typed_value_store.cpp b/src/storage/model/typed_value_store.cpp index 5f4d9bae2..145c92f68 100644 --- a/src/storage/model/typed_value_store.cpp +++ b/src/storage/model/typed_value_store.cpp @@ -10,6 +10,39 @@ const TypedValue& TypedValueStore::at(const TKey &key) const { return TypedValue::Null; } +template +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(const TKey &key, const std::string &value); +template void TypedValueStore::set(const TKey &key, const bool &value); +template void TypedValueStore::set(const TKey &key, const int &value); +template void TypedValueStore::set(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 &kv){return kv.first == key;}); if (found != props_.end()) { @@ -33,3 +66,5 @@ void TypedValueStore::Accept(std::function(), true); EXPECT_EQ(TypedValue(false).Value(), false); - EXPECT_EQ(TypedValue(std::string("bla")).Value(), "bla"); - EXPECT_EQ(TypedValue("bla2").Value(), "bla2"); + EXPECT_EQ(TypedValue(std::string("bla")).Value(), "bla"); + EXPECT_EQ(TypedValue("bla2").Value(), "bla2"); EXPECT_EQ(TypedValue(55).Value(), 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(), 5); EXPECT_FLOAT_EQ((TypedValue(2.5f) + TypedValue(1.25f)).Value(), 3.75); - EXPECT_EQ((TypedValue("one") + TypedValue("two")).Value(), "onetwo"); + EXPECT_EQ((TypedValue("one") + TypedValue("two")).Value(), "onetwo"); // sum of string and numbers - EXPECT_EQ((TypedValue("one") + TypedValue(1)).Value(), "one1"); - EXPECT_EQ((TypedValue(1) + TypedValue("one")).Value(), "1one"); - EXPECT_EQ((TypedValue("one") + TypedValue(3.2f)).Value(), "one3.2"); - EXPECT_EQ((TypedValue(3.2f) + TypedValue("one")).Value(), "3.2one"); + EXPECT_EQ((TypedValue("one") + TypedValue(1)).Value(), "one1"); + EXPECT_EQ((TypedValue(1) + TypedValue("one")).Value(), "1one"); + EXPECT_EQ((TypedValue("one") + TypedValue(3.2f)).Value(), "one3.2"); + EXPECT_EQ((TypedValue(3.2f) + TypedValue("one")).Value(), "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 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