From 6668ecd1701e74305927ddebfe8bdc9712ef741d Mon Sep 17 00:00:00 2001 From: Bernd Amend Date: Thu, 19 Aug 2021 21:10:53 +0200 Subject: [PATCH] Avoid unexpected behavior if move/copy is incorrectly implemented ChaiScript 6 always used a shared_ptr to wrap C++ objects created in ChaiScript. Newer versions avoid this by only wrapping objects that cannot be copied. This has the side effect that ChaiScript depends on correctly implemented C++ copy and move constructors in unexpected locations. I don't think the reduced overhead (by avoiding the shared_ptr) justifies the new behavior in every case. Therefore, I suggest we temporarily only perform this optimization if the class is trivially destructible, until unexpected copies and moves in ChaiScript are fixed (if this is even possible), or there is a sanitizer/compiler warning that can detect these cases. Before the change the following code will call the ctor once, move once, and the dtor twice: auto obj = MoveableObject(); After this change it will only call the ctor once and the dtor once. --- .../dispatchkit/proxy_constructors.hpp | 7 +- unittests/compiled_tests.cpp | 97 ++++++++++++------- 2 files changed, 67 insertions(+), 37 deletions(-) diff --git a/include/chaiscript/dispatchkit/proxy_constructors.hpp b/include/chaiscript/dispatchkit/proxy_constructors.hpp index b7e0d44a..99b1dfe4 100644 --- a/include/chaiscript/dispatchkit/proxy_constructors.hpp +++ b/include/chaiscript/dispatchkit/proxy_constructors.hpp @@ -15,14 +15,17 @@ namespace chaiscript::dispatch::detail { template Proxy_Function build_constructor_(Class (*)(Params...)) { - if constexpr (!std::is_copy_constructible_v) { + // is_copy_constructible is required to avoid compilation errors if classes cannot be copied + // is_trivially_destructible is required to avoid bugs if classes don't correctly implement + // copy or move. To avoid unexpected runtime behavior we allocate them as shared_ptrs. + if constexpr (!std::is_copy_constructible_v || !std::is_trivially_destructible_v) { auto call = [](auto &&...param) { return std::make_shared(std::forward(param)...); }; return Proxy_Function( chaiscript::make_shared(Params...), decltype(call)>>(call)); } else if constexpr (true) { - auto call = [](auto &&...param) { return Class(std::forward(param)...); }; + auto call = [](auto &&...param) { return Class{std::forward(param)...}; }; return Proxy_Function( chaiscript::make_shared>( diff --git a/unittests/compiled_tests.cpp b/unittests/compiled_tests.cpp index f8873df8..b03d08fe 100644 --- a/unittests/compiled_tests.cpp +++ b/unittests/compiled_tests.cpp @@ -568,48 +568,71 @@ TEST_CASE("Utility_Test utility class wrapper for enum") { ////// Object copy count test -class Object_Copy_Count_Test { -public: - Object_Copy_Count_Test() { - std::cout << "Object_Copy_Count_Test()\n"; - ++constructcount(); - } +struct Object_Copy_Count_Test_State { + int ctor; + int copy_ctor; + int move_ctor; + int copy; + int move; + int dtor; +}; - Object_Copy_Count_Test(const Object_Copy_Count_Test &) { - std::cout << "Object_Copy_Count_Test(const Object_Copy_Count_Test &)\n"; - ++copycount(); - } +bool operator==(const Object_Copy_Count_Test_State &s1, const Object_Copy_Count_Test_State &s2) { + return std::make_tuple(s1.ctor, s1.copy_ctor, s1.move_ctor, s1.copy, s1.move, s1.dtor) == std::make_tuple(s2.ctor, s2.copy_ctor, s2.move_ctor, s2.copy, s2.move, s2.dtor); +} - Object_Copy_Count_Test(Object_Copy_Count_Test &&) { - std::cout << "Object_Copy_Count_Test(Object_Copy_Count_Test &&)\n"; - ++movecount(); - } +std::ostream &operator<<(std::ostream &o, const Object_Copy_Count_Test_State &state) { + o << "ctor: " << state.ctor << " copy_ctor: " << state.copy_ctor << " move_ctor: " << state.move_ctor << " copy: " << state.copy << " move: " << state.move << " dtor: " << state.dtor << "\n"; + return o; +} + +struct Object_Copy_Count_Test { + Object_Copy_Count_Test() { state.ctor++; } ~Object_Copy_Count_Test() { - std::cout << "~Object_Copy_Count_Test()\n"; - ++destructcount(); + state.dtor++; } - static int &constructcount() { - static int c = 0; - return c; + Object_Copy_Count_Test(Object_Copy_Count_Test &&other) { + std::swap(state, other.state); + state.move_ctor++; + } + Object_Copy_Count_Test &operator=(Object_Copy_Count_Test &&other) { + std::swap(state, other.state); + state.move++; + return *this; } - static int ©count() { - static int c = 0; - return c; + Object_Copy_Count_Test(const Object_Copy_Count_Test &other) { + state = other.state; + state.copy_ctor++; + } + Object_Copy_Count_Test &operator=(const Object_Copy_Count_Test &other) { + state = other.state; + state.copy++; + return *this; } - static int &movecount() { - static int c = 0; - return c; - } - - static int &destructcount() { - static int c = 0; - return c; - } + static Object_Copy_Count_Test_State state; }; +Object_Copy_Count_Test_State Object_Copy_Count_Test::state = {}; + +TEST_CASE("Test if objects are only copied if necessary") { + chaiscript::ChaiScript_Basic chai(create_chaiscript_stdlib(), create_chaiscript_parser()); + chaiscript::utility::add_class(chai, + "Object_Copy_Count_Test", + {chaiscript::constructor()}, + {}); + + Object_Copy_Count_Test::state = {}; + { + Object_Copy_Count_Test_State state{}; + chai.eval("{auto obj = Object_Copy_Count_Test();}"); + state.ctor = 1; + state.dtor = 1; + CHECK(state == Object_Copy_Count_Test::state); + } +} Object_Copy_Count_Test object_copy_count_create() { return Object_Copy_Count_Test(); @@ -626,12 +649,16 @@ TEST_CASE("Object copy counts") { chaiscript::ChaiScript_Basic chai(create_chaiscript_stdlib(), create_chaiscript_parser()); chai.add(m); + Object_Copy_Count_Test::state = {}; chai.eval(" { auto i = create(); } "); - CHECK(Object_Copy_Count_Test::copycount() == 0); - CHECK(Object_Copy_Count_Test::constructcount() == 1); - CHECK(Object_Copy_Count_Test::destructcount() == 2); - CHECK(Object_Copy_Count_Test::movecount() == 1); + { + Object_Copy_Count_Test_State state{}; + state.ctor = 1; + state.move_ctor = 1; + state.dtor = 2; + CHECK(state == Object_Copy_Count_Test::state); + } } ///////////////////// Object lifetime test 1