From 34bc3d30bce200d3df2adc61990a6090aed6ce02 Mon Sep 17 00:00:00 2001 From: Joshua Scott Date: Tue, 27 Nov 2018 15:36:57 +0000 Subject: [PATCH] etc: Move the responsibility of type checking to properties --- include/ki/pclass/Property.h | 2 + include/ki/pclass/StaticProperty.h | 34 ++++-- include/ki/pclass/VectorProperty.h | 138 +++++++++++++++++-------- include/ki/pclass/types/Type.h | 12 +++ src/pclass/Property.cpp | 7 ++ src/pclass/Type.cpp | 30 ++++++ src/serialization/SerializerBinary.cpp | 30 ------ 7 files changed, 172 insertions(+), 81 deletions(-) diff --git a/include/ki/pclass/Property.h b/include/ki/pclass/Property.h index cd54a7b..a186e81 100644 --- a/include/ki/pclass/Property.h +++ b/include/ki/pclass/Property.h @@ -24,6 +24,7 @@ namespace pclass PropertyBase(PropertyClass &object, const PropertyBase &that); + const PropertyClass &get_instance() const; std::string get_name() const; hash_t get_name_hash() const; hash_t get_full_hash() const; @@ -40,6 +41,7 @@ namespace pclass virtual void read_value_from(BitStream &stream) = 0; private: + const PropertyClass *m_instance; std::string m_name; hash_t m_name_hash; hash_t m_full_hash; diff --git a/include/ki/pclass/StaticProperty.h b/include/ki/pclass/StaticProperty.h index 5ee90ea..611dc69 100644 --- a/include/ki/pclass/StaticProperty.h +++ b/include/ki/pclass/StaticProperty.h @@ -12,8 +12,8 @@ namespace pclass /// @cond DOXYGEN_SKIP /** - * A helper utility that provides the right implementation of construct() - * and get_object() based on characteristics of type: ValueT. + * A helper utility that provides the right implementation of construct(), + * copy(), get_object() and set_object(), based on characteristics of type: ValueT. */ template < typename ValueT, @@ -61,8 +61,9 @@ namespace pclass * - does not derive from PropertyClass * * This should: - * - Construct to a nullptr; and - * - Throw an exception when get_object() is called. + * - Construct to a nullptr; + * - Copy construct to the same pointer as the original; and + * - Throw an exception when get_object() or set_object() is called. */ template struct value_object_helper< @@ -87,7 +88,7 @@ namespace pclass static ValueT copy(const StaticProperty &prop) { // The copy constructor for all pointers is to copy the pointer - // without creating a new copy of the object it's pointing too. + // without creating a new copy of the object it's pointing to. return prop.m_value; } @@ -116,9 +117,12 @@ namespace pclass * - does derive from PropertyClass * * This should: - * - Construct to a nullptr; and + * - Construct to a nullptr; + * - Copy construct to the same pointer as the original; * - Return a pointer to a ValueT instance (as a PropertyClass *) - * when get_object() is called. + * when get_object() is called; and + * - Throw an exception when the object supplied to set_object() + * does not inherit from the property's type, but set m_value otherwise. */ template struct value_object_helper< @@ -143,7 +147,7 @@ namespace pclass static ValueT copy(const StaticProperty &prop) { // The copy constructor for all pointers is to copy the pointer - // without creating a new copy of the object it's pointing too. + // without creating a new copy of the object it's pointing to. return prop.m_value; } @@ -156,6 +160,10 @@ namespace pclass static void set_object(StaticProperty &prop, PropertyClass *object) { + // Ensure that object inherits the type of the property + if (object) + assert_type_match(prop.get_type(), object->get_type(), true); + // ValueT does derive from PropertyClass, and we have a pointer to an instance // of PropertyClass, so cast the pointer up to a ValueT. prop.m_value = dynamic_cast(object); @@ -198,7 +206,7 @@ namespace pclass static ValueT copy(const StaticProperty &prop) { // Derivitives of PropertyClass implement a clone method that returns - // a clone as a pointer. + // a pointer to a copy. ValueT *value_ptr = dynamic_cast(prop.m_value.clone()); ValueT value = *value_ptr; delete value_ptr; @@ -214,9 +222,17 @@ namespace pclass static void set_object(StaticProperty &prop, PropertyClass *object) { + // Ensure that object is not nullptr + if (!object) + throw runtime_error("Value cannot be null."); + + // Ensure that object is exactly the type of the property. + assert_type_match(prop.get_type(), object->get_type()); + // ValueT does derive from PropertyClass, but we don't store a pointer, // so we need to copy the value in. prop.m_value = *object; + delete object; } }; diff --git a/include/ki/pclass/VectorProperty.h b/include/ki/pclass/VectorProperty.h index cbec460..9efd761 100644 --- a/include/ki/pclass/VectorProperty.h +++ b/include/ki/pclass/VectorProperty.h @@ -13,7 +13,8 @@ namespace pclass /// @cond DOXYGEN_SKIP /** - * + * A helper utility that provides the right implementation of copy(), + * get_object() and set_object(), based on characteristics of type: ValueT. */ template < typename ValueT, @@ -24,6 +25,12 @@ namespace pclass { static ValueT copy(VectorProperty &prop, const int index) { + // Ensure index is within bounds + if (index < 0 || index >= prop.size()) + throw runtime_error("Index out of bounds."); + + // In cases where ValueT is not a pointer, and does not derive from PropertyClass, + // just call the copy constructor. return ValueT(prop.at(index)); } @@ -65,6 +72,12 @@ namespace pclass { static ValueT copy(VectorProperty &prop, const int index) { + // Ensure index is within bounds + if (index < 0 || index >= prop.size()) + throw runtime_error("Index out of bounds."); + + // The copy constructor for all pointers is to copy the pointer + // without creating a new copy of the object it's pointing to. return prop.at(index); } @@ -87,47 +100,6 @@ namespace pclass } }; - /** - * - */ - template - struct vector_value_object_helper< - ValueT, - typename std::enable_if< - !std::is_pointer::value - >::type, - typename std::enable_if< - std::is_base_of< - PropertyClass, - typename std::remove_pointer::type - >::value - >::type - > - { - static ValueT copy(VectorProperty &prop, const int index) - { - ValueT *value_ptr = dynamic_cast(prop.at(index).copy()); - ValueT value = *value_ptr; - delete value_ptr; - return value; - } - - static const PropertyClass *get_object(const VectorProperty &prop, const int index) - { - if (index < 0 || index >= prop.size()) - throw runtime_error("Index out of bounds."); - return dynamic_cast(&prop.at(index)); - } - - static void set_object(VectorProperty &prop, PropertyClass *object, const int index) - { - if (index < 0 || index >= prop.size()) - throw runtime_error("Index out of bounds."); - prop.at(index) = dynamic_cast(*object); - delete object; - } - }; - /** * */ @@ -147,24 +119,100 @@ namespace pclass { static ValueT copy(VectorProperty &prop, const int index) { + // The copy constructor for all pointers is to copy the pointer + // without creating a new copy of the object it's pointing to. return prop.at(index); } static const PropertyClass *get_object(const VectorProperty &prop, const int index) { + // Ensure index is within bounds if (index < 0 || index >= prop.size()) throw runtime_error("Index out of bounds."); + + // ValueT does derive from PropertyClass, and we have a pointer to an instance + // of ValueT, so we can cast down to a PropertyClass pointer. return dynamic_cast(prop.at(index)); } static void set_object(VectorProperty &prop, PropertyClass *object, const int index) { + // Ensure index is within bounds if (index < 0 || index >= prop.size()) throw runtime_error("Index out of bounds."); + + // Ensure that object inherits the type of the property + if (object) + assert_type_match(prop.get_type(), object->get_type(), true); + + // ValueT does derive from PropertyClass, and we have a pointer to an instance + // of PropertyClass, so cast the pointer up to a ValueT. prop.at(index) = dynamic_cast(object); } }; + /** + * + */ + template + struct vector_value_object_helper< + ValueT, + typename std::enable_if< + !std::is_pointer::value + >::type, + typename std::enable_if< + std::is_base_of< + PropertyClass, + typename std::remove_pointer::type + >::value + >::type + > + { + static ValueT copy(VectorProperty &prop, const int index) + { + // Ensure index is within bounds + if (index < 0 || index >= prop.size()) + throw runtime_error("Index out of bounds."); + + // Derivitives of PropertyClass implement a clone method that returns + // a pointer to a copy. + ValueT *value_ptr = dynamic_cast(prop.at(index).copy()); + ValueT value = *value_ptr; + delete value_ptr; + return value; + } + + static const PropertyClass *get_object(const VectorProperty &prop, const int index) + { + // Ensure index is within bounds + if (index < 0 || index >= prop.size()) + throw runtime_error("Index out of bounds."); + + // ValueT does derive from PropertyClass, and we have an instance of ValueT, + // so we can cast down to a PropertyClass pointer. + return dynamic_cast(&prop.at(index)); + } + + static void set_object(VectorProperty &prop, PropertyClass *object, const int index) + { + // Ensure index is within bounds + if (index < 0 || index >= prop.size()) + throw runtime_error("Index out of bounds."); + + // Ensure that object is not nullptr + if (!object) + throw runtime_error("Value cannot be null."); + + // Ensure that object is exactly the type of the property + assert_type_match(prop.get_type(), object->get_type()); + + // ValueT does derive from PropertyClass, but we don't store a pointer, + // so we need to copy the value in. + prop.at(index) = dynamic_cast(*object); + delete object; + } + }; + /** * */ @@ -176,6 +224,7 @@ namespace pclass { static void write_value_to(const VectorProperty &prop, BitStream &stream, const int index) { + // Ensure index is within bounds if (index < 0 || index >= prop.size()) throw runtime_error("Index out of bounds."); prop.get_type().write_to(stream, prop.at(index)); @@ -183,6 +232,7 @@ namespace pclass static void read_value_from(VectorProperty &prop, BitStream &stream, const int index) { + // Ensure index is within bounds if (index < 0 || index >= prop.size()) throw runtime_error("Index out of bounds."); prop.get_type().read_from(stream, Value(prop.at(index))); @@ -200,15 +250,19 @@ namespace pclass { static void write_value_to(const VectorProperty &prop, BitStream &stream, const int index) { + // Ensure index is within bounds if (index < 0 || index >= prop.size()) throw runtime_error("Index out of bounds."); + prop.get_type().write_to(stream, *prop.at(index)); } static void read_value_from(VectorProperty &prop, BitStream &stream, const int index) { + // Ensure index is within bounds if (index < 0 || index >= prop.size()) throw runtime_error("Index out of bounds."); + prop.get_type().read_from(stream, Value(*prop.at(index))); } }; diff --git a/include/ki/pclass/types/Type.h b/include/ki/pclass/types/Type.h index eaf3529..40438df 100644 --- a/include/ki/pclass/types/Type.h +++ b/include/ki/pclass/types/Type.h @@ -71,5 +71,17 @@ namespace pclass typedef std::vector TypeList; typedef std::map TypeNameMap; typedef std::map TypeHashMap; + + /** + * @param[in] expected The Type that is expected to match with the actual Type. + * @param[in] actual The Type that is being checked for a match with the expected Type. + * @param[in] allow_inheritance If false, then the expected and actual types must match exactly. + * @throws ki::runtime_error If the expected and actual types do not match. + */ + void assert_type_match( + const Type &expected, + const Type &actual, + bool allow_inheritance = false + ); } } diff --git a/src/pclass/Property.cpp b/src/pclass/Property.cpp index a8e5688..4d81e18 100644 --- a/src/pclass/Property.cpp +++ b/src/pclass/Property.cpp @@ -10,6 +10,7 @@ namespace pclass PropertyBase::PropertyBase(PropertyClass &object, const std::string &name, const Type &type) { + m_instance = &object; m_name = name; m_name_hash = type .get_type_system() @@ -25,6 +26,7 @@ namespace pclass PropertyBase::PropertyBase(PropertyClass &object, const PropertyBase &that) { + m_instance = &object; m_name = that.m_name; m_name_hash = that.m_name_hash; m_full_hash = that.m_full_hash; @@ -34,6 +36,11 @@ namespace pclass object.add_property(*this); } + const PropertyClass &PropertyBase::get_instance() const + { + return *m_instance; + } + std::string PropertyBase::get_name() const { return m_name; diff --git a/src/pclass/Type.cpp b/src/pclass/Type.cpp index 56560ca..76e877e 100644 --- a/src/pclass/Type.cpp +++ b/src/pclass/Type.cpp @@ -1,4 +1,5 @@ #include "ki/pclass/types/Type.h" +#include "ki/pclass/types/ClassType.h" #include "ki/pclass/TypeSystem.h" #include "ki/util/exception.h" #include @@ -65,5 +66,34 @@ namespace pclass .get_hash_calculator() .calculate_type_hash(m_name); } + + void assert_type_match( + const Type &expected, + const Type &actual, + const bool allow_inheritance + ) + { + // Do the types match via inheritance? + if (allow_inheritance && + expected.get_kind() == Type::kind::CLASS) + { + const auto &actual_class = dynamic_cast(actual); + if (actual_class.inherits(expected)) + return; + } + + // Do the types match exactly? + if (&expected == &actual) + return; + + // The types do not match + std::ostringstream oss; + oss << "Type mismatch. (" + << "expected=" << expected.get_name() << ", " + << "actual=" << actual.get_name() << ", " + << "allow_inheritance=" << allow_inheritance + << ")"; + throw runtime_error(oss.str()); + } } } diff --git a/src/serialization/SerializerBinary.cpp b/src/serialization/SerializerBinary.cpp index 52ec7f8..8028353 100644 --- a/src/serialization/SerializerBinary.cpp +++ b/src/serialization/SerializerBinary.cpp @@ -404,21 +404,6 @@ namespace serialization // Read the object as a nested object pclass::PropertyClass *object = nullptr; load_object(object, stream); - if (object) - { - // Does the nested object inherit from the property's type? - const auto &object_type = - dynamic_cast( - object->get_type() - ); - if (!object_type.inherits(dynamic_property.get_type())) - { - std::ostringstream oss; - oss << "Type '" << object_type.get_name() << "' does not derive from " - << "property type '" << prop.get_type().get_name() << "'."; - throw runtime_error(oss.str()); - } - } dynamic_property.set_object(object, i); } else @@ -438,21 +423,6 @@ namespace serialization // Read the object as a nested object pclass::PropertyClass *object = nullptr; load_object(object, stream); - if (object) - { - // Does the nested object inherit from the property's type? - const auto &object_type = - dynamic_cast( - object->get_type() - ); - if (!object_type.inherits(prop.get_type())) - { - std::ostringstream oss; - oss << "Type '" << object_type.get_name() << "' does not derive from " - << "property type '" << prop.get_type().get_name() << "'."; - throw runtime_error(oss.str()); - } - } prop.set_object(object); } else