Skip to content

Avoid unexpected behavior if move/copy is incorrectly implemented #577

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
BerndAmend wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Avoid unexpected behavior if move/copy is incorrectly implemented #577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions include/chaiscript/dispatchkit/proxy_constructors.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)>>(
Expand Down
95 changes: 61 additions & 34 deletions unittests/compiled_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
}

static int &copycount() {
static int c = 0;
return c;
Object_Copy_Count_Test &operator=(Object_Copy_Count_Test &&other) {
std::swap(state, other.state);
state.move++;
return *this;
}

static int &movecount() {
static int c = 0;
return c;
Object_Copy_Count_Test(const Object_Copy_Count_Test &other) {
state = other.state;
state.copy_ctor++;
}

static int &destructcount() {
static int c = 0;
return c;
Object_Copy_Count_Test &operator=(const Object_Copy_Count_Test &other) {
state = other.state;
state.copy++;
return *this;
}

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();
Expand All @@ -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
Expand Down