From 972b93f95b5cfdc68e914bea4bcf9dbb037ae23c Mon Sep 17 00:00:00 2001 From: Dan Goodliffe Date: Thu, 21 Sep 2023 20:08:21 +0100 Subject: Remove void * in class hierarchy handling Replaces the map of typename to function pointer taking void * (which pointed to the std::shared_ptr and cast that to a shared_ptr, thus invoking undefined behaviour) with a map to a small class hierarchy which represents that matching the model class and applying dynamic casts to properly convert as required. --- slicer/slicer/modelParts.h | 7 +++++- slicer/slicer/modelPartsTypes.cpp | 10 ++++----- slicer/slicer/modelPartsTypes.h | 15 +++++++++---- slicer/slicer/modelPartsTypes.impl.h | 43 ++++++++++++++++++++++++++++-------- slicer/tool/parser.cpp | 10 +++++++++ 5 files changed, 66 insertions(+), 19 deletions(-) diff --git a/slicer/slicer/modelParts.h b/slicer/slicer/modelParts.h index 98de0d6..d34552e 100644 --- a/slicer/slicer/modelParts.h +++ b/slicer/slicer/modelParts.h @@ -77,6 +77,12 @@ namespace Slicer { class ModelPartForRootBase; class HookCommon; + struct ClassRefBase { + constexpr ClassRefBase() = default; + virtual ~ClassRefBase() = default; + SPECIAL_MEMBERS_DEFAULT(ClassRefBase); + }; + using ModelPartParam = any_ptr; using ModelPartForRootParam = any_ptr; using TypeId = std::optional; @@ -85,7 +91,6 @@ namespace Slicer { using ModelPartHandler = std::function; using ModelPartRootHandler = std::function; using SubPartHandler = std::function; - using ClassRef = std::function; using HookFilter = std::function; constexpr Metadata emptyMetadata; diff --git a/slicer/slicer/modelPartsTypes.cpp b/slicer/slicer/modelPartsTypes.cpp index e78c6c6..c57d987 100644 --- a/slicer/slicer/modelPartsTypes.cpp +++ b/slicer/slicer/modelPartsTypes.cpp @@ -27,7 +27,7 @@ namespace Ice { } namespace Slicer { - using ClassRefMap = std::map>; + using ClassRefMap = std::map>; using ClassNamePair = std::pair; using ClassNameMap = boost::multi_index_container typeName, const ClassRef & cr) + const std::string_view className, const std::optional typeName, const ClassRefBase * cr) { refs->emplace(className, cr); if (typeName) { @@ -206,11 +206,11 @@ namespace Slicer { } } - void - ModelPartForComplexBase::onSubclass(const std::string & name, void * m, const ModelPartHandler & h) + const ClassRefBase * + ModelPartForComplexBase::getSubclassRef(const std::string & name) { if (const auto ref = refs->find(ToModelTypeName(name)); ref != refs->end()) { - return ref->second(m, h); + return ref->second; } throw UnknownType(name); } diff --git a/slicer/slicer/modelPartsTypes.h b/slicer/slicer/modelPartsTypes.h index b30636e..6b6f852 100644 --- a/slicer/slicer/modelPartsTypes.h +++ b/slicer/slicer/modelPartsTypes.h @@ -137,10 +137,10 @@ namespace Slicer { static const ModelPartType type; protected: - void onSubclass(const std::string & name, void * m, const ModelPartHandler &); + const ClassRefBase * getSubclassRef(const std::string & name); static void registerClass( - const std::string_view className, const std::optional typeName, const ClassRef &); + const std::string_view className, const std::optional typeName, const ClassRefBase *); static void unregisterClass(const std::string_view className, const std::optional typeName); static TypeId getTypeId(const std::string & id, const std::string_view className); static std::string demangle(const char * mangled); @@ -196,13 +196,20 @@ namespace Slicer { constinit static const std::string_view className; constinit static const std::optional typeName; - static void CreateModelPart(void *, const ModelPartHandler &); - private: + static const ClassRefBase * const classref; static void registerClass() __attribute__((constructor(210))); static void unregisterClass() __attribute__((destructor(210))); }; + template struct ClassRef { + consteval ClassRef() = default; + virtual ~ClassRef() = default; + SPECIAL_MEMBERS_DEFAULT(ClassRef); + + virtual void onSubClass(std::shared_ptr &, const ModelPartHandler &) const = 0; + }; + template class ModelPartForStruct : public ModelPartForComplex, protected ModelPartModel { public: using element_type = T; diff --git a/slicer/slicer/modelPartsTypes.impl.h b/slicer/slicer/modelPartsTypes.impl.h index f3d1a42..3d1d2be 100644 --- a/slicer/slicer/modelPartsTypes.impl.h +++ b/slicer/slicer/modelPartsTypes.impl.h @@ -448,7 +448,12 @@ namespace Slicer { ModelPartForClass::OnSubclass(const ModelPartHandler & h, const std::string & name) { BOOST_ASSERT(this->Model); - return ModelPartForComplexBase::onSubclass(name, this->Model, h); + const ClassRefBase * refbase = ModelPartForComplexBase::getSubclassRef(name); + BOOST_ASSERT(refbase); + BOOST_ASSERT(dynamic_cast *>(refbase)); + if (auto ref = dynamic_cast *>(refbase)) { + ref->onSubClass(*this->Model, h); + } } template @@ -466,20 +471,40 @@ namespace Slicer { return typeIdProperty; } - template - void - ModelPartForClass::CreateModelPart(void * p, const ModelPartHandler & h) - { - return ::Slicer::ModelPart::CreateFor(static_cast(p), h); - } - template void ModelPartForClass::registerClass() { - ModelPartForComplexBase::registerClass(className, typeName, &ModelPartForClass::CreateModelPart); + ModelPartForComplexBase::registerClass(className, typeName, classref); } + template struct ClassRefImpl : public ClassRef { + consteval ClassRefImpl() = default; + + void + onSubClass(std::shared_ptr & model, const ModelPartHandler & h) const override + { + if constexpr (std::is_same_v) { + ModelPart::CreateFor(&model, h); + } + else { + auto p = std::move(std::dynamic_pointer_cast(model)); + ModelPart::CreateFor(&p, h); + model = std::move(p); + } + } + }; + + template + struct ClassRefImplImpl : + public ClassRefBase, + public ClassRefImpl, + public ClassRefImpl... { + consteval ClassRefImplImpl() = default; + ~ClassRefImplImpl() override = default; + SPECIAL_MEMBERS_DEFAULT(ClassRefImplImpl); + }; + template void ModelPartForClass::unregisterClass() diff --git a/slicer/tool/parser.cpp b/slicer/tool/parser.cpp index 445dda5..8a58484 100644 --- a/slicer/tool/parser.cpp +++ b/slicer/tool/parser.cpp @@ -415,6 +415,16 @@ namespace Slicer { auto name = md.value("slicer:root:"); defineRoot(typeToString(decl), name ? *name : c->name(), decl); + fprintbf(cpp, "static constexpr ClassRefImplImpl<%s", decl->typeId()); + for (const auto & base : decl->definition()->allBases()) { + if (decl != base->declaration()) { + fprintbf(cpp, ", %s", base->declaration()->typeId()); + } + } + fprintbf(cpp, "> ref%d;\n", components); + fprintbf(cpp, "template<>\n"); + fprintbf(cpp, "constinit const ClassRefBase * const ModelPartForClass<%s>::classref{ &ref%d };\n", + decl->typeId(), components); auto typeName = md.value("slicer:typename:"); fprintbf(cpp, "template<>\n"); -- cgit v1.2.3 From 103dc92125951019e8948026713810bbfd649db5 Mon Sep 17 00:00:00 2001 From: Dan Goodliffe Date: Thu, 21 Sep 2023 20:37:37 +0100 Subject: Add missing dependencies on test files --- slicer/test/Jamfile.jam | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/slicer/test/Jamfile.jam b/slicer/test/Jamfile.jam index 4ed71fb..20cc95e 100644 --- a/slicer/test/Jamfile.jam +++ b/slicer/test/Jamfile.jam @@ -1,4 +1,5 @@ import ./slicer.jam ; +import sequence ; lib dl ; lib stdc++fs ; @@ -81,7 +82,7 @@ run compilation.cpp run serializers.cpp helpers.cpp - : : : + : -- : [ sequence.insertion-sort [ glob-tree-ex initial included expected : *.json *.xml ] ] : types types common @@ -128,7 +129,7 @@ run ../xml//slicer-xml ../json//slicer-json ] - : : : + : -- : [ sequence.insertion-sort [ glob-tree-ex initial included expected : *.json *.xml ] ] : benchmark stdc++fs common -- cgit v1.2.3 From 5269827c4f5ef6c07bef4719cff818b19ebde853 Mon Sep 17 00:00:00 2001 From: Dan Goodliffe Date: Sun, 24 Sep 2023 13:28:09 +0100 Subject: Test coverage and fix error handling of invalid types --- slicer/slicer/common.ice | 5 +++++ slicer/slicer/modelPartsTypes.cpp | 6 ++++++ slicer/slicer/modelPartsTypes.h | 1 + slicer/slicer/modelPartsTypes.impl.h | 9 ++++---- slicer/slicer/slicer.cpp | 8 +++++++ slicer/test/initial/inherit-base.xml | 4 ++++ slicer/test/initial/inherit-nosuchtype.xml | 5 +++++ slicer/test/initial/inherit-same.xml | 4 ++++ slicer/test/initial/inherit-wronghier.xml | 5 +++++ slicer/test/serializers.cpp | 34 +++++++++++++++++++++++++++++- 10 files changed, 76 insertions(+), 5 deletions(-) create mode 100644 slicer/test/initial/inherit-base.xml create mode 100644 slicer/test/initial/inherit-nosuchtype.xml create mode 100644 slicer/test/initial/inherit-same.xml create mode 100644 slicer/test/initial/inherit-wronghier.xml diff --git a/slicer/slicer/common.ice b/slicer/slicer/common.ice index 60a4f92..ad3ff1f 100644 --- a/slicer/slicer/common.ice +++ b/slicer/slicer/common.ice @@ -29,6 +29,11 @@ module Slicer { string type; }; ["cpp:ice_print"] + exception IncorrectType extends DeserializerError { + string type; + string target; + }; + ["cpp:ice_print"] exception InvalidEnumerationValue extends SerializerError { int value; string type; diff --git a/slicer/slicer/modelPartsTypes.cpp b/slicer/slicer/modelPartsTypes.cpp index c57d987..ffaec04 100644 --- a/slicer/slicer/modelPartsTypes.cpp +++ b/slicer/slicer/modelPartsTypes.cpp @@ -74,6 +74,12 @@ namespace Slicer { return name; } + [[noreturn]] void + ModelPartForComplexBase::throwIncorrectType(const std::string & name, const std::type_info & target) + { + throw IncorrectType(name, demangle(target.name())); + } + #define Roots(Type, Name, NameLen) \ template<> CONSTSTR(NameLen) Slicer::ModelPartForRoot::rootName {#Name}; \ template<> \ diff --git a/slicer/slicer/modelPartsTypes.h b/slicer/slicer/modelPartsTypes.h index 6b6f852..f581c49 100644 --- a/slicer/slicer/modelPartsTypes.h +++ b/slicer/slicer/modelPartsTypes.h @@ -147,6 +147,7 @@ namespace Slicer { static const std::string & ToExchangeTypeName(const std::string &); static std::string_view ToModelTypeName(const std::string &); + [[noreturn]] static void throwIncorrectType(const std::string & name, const std::type_info & target); }; template class Hooks; diff --git a/slicer/slicer/modelPartsTypes.impl.h b/slicer/slicer/modelPartsTypes.impl.h index 3d1d2be..60f221d 100644 --- a/slicer/slicer/modelPartsTypes.impl.h +++ b/slicer/slicer/modelPartsTypes.impl.h @@ -448,12 +448,13 @@ namespace Slicer { ModelPartForClass::OnSubclass(const ModelPartHandler & h, const std::string & name) { BOOST_ASSERT(this->Model); - const ClassRefBase * refbase = ModelPartForComplexBase::getSubclassRef(name); - BOOST_ASSERT(refbase); - BOOST_ASSERT(dynamic_cast *>(refbase)); - if (auto ref = dynamic_cast *>(refbase)) { + if (const ClassRefBase * refbase = ModelPartForComplexBase::getSubclassRef(name); + auto ref = dynamic_cast *>(refbase)) [[likely]] { ref->onSubClass(*this->Model, h); } + else { + ModelPartForComplexBase::throwIncorrectType(name, typeid(T)); + } } template diff --git a/slicer/slicer/slicer.cpp b/slicer/slicer/slicer.cpp index ff9a948..5a253b0 100644 --- a/slicer/slicer/slicer.cpp +++ b/slicer/slicer/slicer.cpp @@ -13,6 +13,14 @@ namespace Slicer { InvalidEnumerationSymbolMsg::write(s, symbol, type); } + AdHocFormatter(IncorrectTypeMsg, "Type [%?] cannot be used as a [%?]"); + + void + IncorrectType::ice_print(std::ostream & s) const + { + IncorrectTypeMsg::write(s, type, target); + } + AdHocFormatter(InvalidEnumerationValueMsg, "Invalid enumeration symbol [%?] for type [%?]"); void diff --git a/slicer/test/initial/inherit-base.xml b/slicer/test/initial/inherit-base.xml new file mode 100644 index 0000000..45456e2 --- /dev/null +++ b/slicer/test/initial/inherit-base.xml @@ -0,0 +1,4 @@ + + + 3 + diff --git a/slicer/test/initial/inherit-nosuchtype.xml b/slicer/test/initial/inherit-nosuchtype.xml new file mode 100644 index 0000000..057bea5 --- /dev/null +++ b/slicer/test/initial/inherit-nosuchtype.xml @@ -0,0 +1,5 @@ + + + 3 + 3 + diff --git a/slicer/test/initial/inherit-same.xml b/slicer/test/initial/inherit-same.xml new file mode 100644 index 0000000..77a22ec --- /dev/null +++ b/slicer/test/initial/inherit-same.xml @@ -0,0 +1,4 @@ + + + 3 + diff --git a/slicer/test/initial/inherit-wronghier.xml b/slicer/test/initial/inherit-wronghier.xml new file mode 100644 index 0000000..d1ce693 --- /dev/null +++ b/slicer/test/initial/inherit-wronghier.xml @@ -0,0 +1,5 @@ + + + 3 + 3 + diff --git a/slicer/test/serializers.cpp b/slicer/test/serializers.cpp index 57779be..ae9a30e 100644 --- a/slicer/test/serializers.cpp +++ b/slicer/test/serializers.cpp @@ -1,4 +1,5 @@ #define BOOST_TEST_MODULE execute_serializers +#include #include #include "classes.h" @@ -36,7 +37,6 @@ #include #include #include -#include #include #include // IWYU pragma: no_forward_declare Slicer::InvalidEnumerationSymbol @@ -677,3 +677,35 @@ BOOST_AUTO_TEST_CASE(sequence_element_in_same_slice_link_bug) BOOST_CHECK_NO_THROW( Slicer::ModelPart::Make>(nullptr, [](auto &&) {})); } + +BOOST_AUTO_TEST_CASE(typeid_specifies_same) +{ + std::ifstream in {rootDir / "initial/inherit-same.xml"}; + auto d1 = Slicer::DeserializeAny(in); + BOOST_REQUIRE(d1); + BOOST_CHECK_EQUAL(d1->a, 3); + BOOST_CHECK_EQUAL(typeid(*d1).name(), typeid(TestModule::D1).name()); +} + +BOOST_DATA_TEST_CASE(typeid_specifies_bad, + boost::unit_test::data::make({ + "inherit-base.xml", + "inherit-wronghier.xml", + }), + path) +{ + std::ifstream in {rootDir / "initial" / path}; + BOOST_CHECK_THROW(std::ignore = (Slicer::DeserializeAny(in)), + Slicer::IncorrectType); +} + +BOOST_DATA_TEST_CASE(typeid_specifies_no_such_type, + boost::unit_test::data::make({ + "inherit-nosuchtype.xml", + }), + path) +{ + std::ifstream in {rootDir / "initial" / path}; + BOOST_CHECK_THROW(std::ignore = (Slicer::DeserializeAny(in)), + Slicer::UnknownType); +} -- cgit v1.2.3