coffee/ChaiScript
1
0
Fork 0
mirror of https://github.com/ChaiScript/ChaiScript.git synced 2025-12-08 01:36:54 +08:00
Code Issues Packages Projects Releases Wiki Activity

Avoid unexpected behavior if move/copy is incorrectly implemented

Browse Source 
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.
This commit is contained in:
Bernd Amend 2021-08-19 21:10:53 +02:00
parent 6411aa7498
commit 6668ecd170
2 changed files with 67 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
include/chaiscript/dispatchkit/proxy_constructors.hpp
View File

@ -15,14 +15,17 @@
namespace chaiscript::dispatch::detail {
template<typename Class, typename... Params>
Proxy_Function build_constructor_(Class (*)(Params...)) {
if constexpr (!std::is_copy_constructible_v<Class>) {
// 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<Class> || !std::is_trivially_destructible_v<Class>) {
auto call = [](auto &&...param) { return std::make_shared<Class>(std::forward<decltype(param)>(param)...); };
return Proxy_Function(
chaiscript::make_shared<dispatch::Proxy_Function_Base,
dispatch::Proxy_Function_Callable_Impl<std::shared_ptr<Class>(Params...), decltype(call)>>(call));
} else if constexpr (true) {
auto call = [](auto &&...param) { return Class(std::forward<decltype(param)>(param)...); };
auto call = [](auto &&...param) { return Class{std::forward<decltype(param)>(param)...}; };
return Proxy_Function(
chaiscript::make_shared<dispatch::Proxy_Function_Base, dispatch::Proxy_Function_Callable_Impl<Class(Params...), decltype(call)>>(

97
unittests/compiled_tests.cpp
View File

@ -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 &copycount() {
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<Object_Copy_Count_Test>(chai,
"Object_Copy_Count_Test",
{chaiscript::constructor<Object_Copy_Count_Test()>()},
{});
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