From 349425fe8a5ae221c49d2cdfe71497bbebbfbe34 Mon Sep 17 00:00:00 2001 From: Jason Turner Date: Sun, 20 May 2012 07:04:22 -0600 Subject: [PATCH 1/4] Make vector inplace construction consistent with map - Clone elements into both vector and map - Be sure to drop dependencies after elements are cloned in --- CMakeLists.txt | 2 +- .../chaiscript/language/chaiscript_eval.hpp | 23 +++++++++++++------ unittests/map_inplace_init.chai | 11 +++++++++ unittests/vector_inplace_init.chai | 12 ++++++++++ 4 files changed, 40 insertions(+), 8 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4d3e07d..65989e4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -185,7 +185,7 @@ if(BUILD_TESTING) add_executable(short_comparison_test unittests/short_comparison_test.cpp) target_link_libraries(short_comparison_test ${LIBS}) - add_test(NAME short_comparison_test COMMAND short_comparison_test) + add_test(NAME Short_Comparison_Test COMMAND short_comparison_test) add_executable(multifile_test unittests/multifile_test_main.cpp unittests/multifile_test_chai.cpp diff --git a/include/chaiscript/language/chaiscript_eval.hpp b/include/chaiscript/language/chaiscript_eval.hpp index 828a041..e542f85 100644 --- a/include/chaiscript/language/chaiscript_eval.hpp +++ b/include/chaiscript/language/chaiscript_eval.hpp @@ -831,14 +831,21 @@ namespace chaiscript AST_Node(t_ast_node_text, t_id, t_fname, t_start_line, t_start_col, t_end_line, t_end_col) { } virtual ~Inline_Array_AST_Node() {} virtual Boxed_Value eval_internal(chaiscript::detail::Dispatch_Engine &t_ss){ - std::vector vec; - if (this->children.size() > 0) { - for (size_t i = 0; i < this->children[0]->children.size(); ++i) { - vec.push_back(this->children[0]->children[i]->eval(t_ss)); + try { + std::vector vec; + if (this->children.size() > 0) { + for (size_t i = 0; i < this->children[0]->children.size(); ++i) { + Boxed_Value bv = t_ss.call_function("clone", this->children[0]->children[i]->eval(t_ss)); + bv.clear_dependencies(); + vec.push_back(bv); + } } + return const_var(vec); + } + catch (const exception::dispatch_error &) { + throw exception::eval_error("Can not find appropriate 'clone' or copy constructor for vector elements"); } - return const_var(vec); } }; @@ -852,13 +859,15 @@ namespace chaiscript try { std::map retval; for (size_t i = 0; i < this->children[0]->children.size(); ++i) { + Boxed_Value bv = t_ss.call_function("clone", this->children[0]->children[i]->children[1]->eval(t_ss)); + bv.clear_dependencies(); retval[boxed_cast(this->children[0]->children[i]->children[0]->eval(t_ss))] - = t_ss.call_function("clone", this->children[0]->children[i]->children[1]->eval(t_ss)); + = bv; } return const_var(retval); } catch (const exception::dispatch_error &) { - throw exception::eval_error("Can not find appropriate 'Map()'"); + throw exception::eval_error("Can not find appropriate 'clone' or copy constructor for map elements"); } } diff --git a/unittests/map_inplace_init.chai b/unittests/map_inplace_init.chai index b1d8221..138ef03 100644 --- a/unittests/map_inplace_init.chai +++ b/unittests/map_inplace_init.chai @@ -1,3 +1,14 @@ var x = ["bob":1, "fred":2] assert_equal(2, x.size()); + + +// Make sure vector elements are copied into place for consistency with +// map inplace construction +var i = 1; +var y = ["a":i]; + +assert_equal(1, y["a"]); +i = 3; +assert_equal(3, i); +assert_equal(1, y["a"]); diff --git a/unittests/vector_inplace_init.chai b/unittests/vector_inplace_init.chai index f16c15b..e7ae124 100644 --- a/unittests/vector_inplace_init.chai +++ b/unittests/vector_inplace_init.chai @@ -1,2 +1,14 @@ var x = [1, 2, 3] assert_equal(3, x.size()) + + + +// Make sure vector elements are copied into place for consistency with +// map inplace construction +var i = 1; +var y = [i]; + +assert_equal(1, y[0]); +i = 3; +assert_equal(3, i); +assert_equal(1, y[0]); From 654f7e6b017c94336fd70be557a89726c41c0607 Mon Sep 17 00:00:00 2001 From: Jason Turner Date: Mon, 21 May 2012 07:56:38 -0600 Subject: [PATCH 2/4] Add unit test exposing how scope can leak into operator calls --- unittests/operator_scoping.chai | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 unittests/operator_scoping.chai diff --git a/unittests/operator_scoping.chai b/unittests/operator_scoping.chai new file mode 100644 index 0000000..e6f9272 --- /dev/null +++ b/unittests/operator_scoping.chai @@ -0,0 +1,14 @@ +def `+`(x, y) +{ + print(i); +} + +auto i = 10; + + +// It should fail because `+` should not be able to see the i + +"1" + 1; +assert_false(true); + + From ef46d1bf60a48f1378f83168a99bacbc3e6bcff7 Mon Sep 17 00:00:00 2001 From: Jason Turner Date: Mon, 21 May 2012 08:18:33 -0600 Subject: [PATCH 3/4] Remove Boxed_Value dependencies, they are not a solution --- .../chaiscript/dispatchkit/boxed_value.hpp | 19 ------------------- .../dispatchkit/proxy_functions.hpp | 1 - .../chaiscript/language/chaiscript_eval.hpp | 3 --- 3 files changed, 23 deletions(-) diff --git a/include/chaiscript/dispatchkit/boxed_value.hpp b/include/chaiscript/dispatchkit/boxed_value.hpp index 25a1e31..59e7f90 100644 --- a/include/chaiscript/dispatchkit/boxed_value.hpp +++ b/include/chaiscript/dispatchkit/boxed_value.hpp @@ -71,7 +71,6 @@ namespace chaiscript void *m_data_ptr; const void *m_const_data_ptr; bool m_is_ref; - std::vector > m_dependencies; }; struct Object_Data @@ -242,24 +241,6 @@ namespace chaiscript return !is_ref(); } - void clear_dependencies() - { - m_data->m_dependencies.clear(); - } - - template - void add_dependencies(InItr begin, const InItr &end) - { - while (begin != end) - { - if (begin->m_data != m_data) - { - m_data->m_dependencies.push_back(begin->m_data); - } - ++begin; - } - } - void *get_ptr() const { return m_data->m_data_ptr; diff --git a/include/chaiscript/dispatchkit/proxy_functions.hpp b/include/chaiscript/dispatchkit/proxy_functions.hpp index 5b642cd..fe47ae5 100644 --- a/include/chaiscript/dispatchkit/proxy_functions.hpp +++ b/include/chaiscript/dispatchkit/proxy_functions.hpp @@ -74,7 +74,6 @@ namespace chaiscript Boxed_Value operator()(const std::vector ¶ms) const { Boxed_Value bv = do_call(params); - bv.add_dependencies(params.begin(), params.end()); return bv; } diff --git a/include/chaiscript/language/chaiscript_eval.hpp b/include/chaiscript/language/chaiscript_eval.hpp index e542f85..66e214f 100644 --- a/include/chaiscript/language/chaiscript_eval.hpp +++ b/include/chaiscript/language/chaiscript_eval.hpp @@ -279,7 +279,6 @@ namespace chaiscript try { if (lhs.is_undef()) { retval = t_ss.call_function("clone", retval); - retval.clear_dependencies(); } try { @@ -836,7 +835,6 @@ namespace chaiscript if (this->children.size() > 0) { for (size_t i = 0; i < this->children[0]->children.size(); ++i) { Boxed_Value bv = t_ss.call_function("clone", this->children[0]->children[i]->eval(t_ss)); - bv.clear_dependencies(); vec.push_back(bv); } } @@ -860,7 +858,6 @@ namespace chaiscript std::map retval; for (size_t i = 0; i < this->children[0]->children.size(); ++i) { Boxed_Value bv = t_ss.call_function("clone", this->children[0]->children[i]->children[1]->eval(t_ss)); - bv.clear_dependencies(); retval[boxed_cast(this->children[0]->children[i]->children[0]->eval(t_ss))] = bv; } From 3a7eff1478d192f7aa3165294aac09a0c58525a6 Mon Sep 17 00:00:00 2001 From: Jason Turner Date: Mon, 21 May 2012 10:16:16 -0600 Subject: [PATCH 4/4] Move to a bit smarter stack based object management - we store all function parameters until the f outer function call exits - this results in more values being stored longer than they need to be, but the results are predictable and no leaks --- .../chaiscript/dispatchkit/dispatchkit.hpp | 27 ++++++++++++++++ .../chaiscript/language/chaiscript_common.hpp | 32 +++++++++++++++++-- .../chaiscript/language/chaiscript_eval.hpp | 6 ++++ 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/include/chaiscript/dispatchkit/dispatchkit.hpp b/include/chaiscript/dispatchkit/dispatchkit.hpp index 4056a2e..9269ff7 100644 --- a/include/chaiscript/dispatchkit/dispatchkit.hpp +++ b/include/chaiscript/dispatchkit/dispatchkit.hpp @@ -897,6 +897,29 @@ namespace chaiscript m_state = t_state; } + void save_function_params(const std::vector &t_params) + { + m_stack_holder->call_params.insert(m_stack_holder->call_params.begin(), t_params.begin(), t_params.end()); + } + + void new_function_call() + { + ++m_stack_holder->call_depth; + } + + void pop_function_call() + { + --m_stack_holder->call_depth; + + assert(m_stack_holder->call_depth >= 0); + + if (m_stack_holder->call_depth == 0) + { + /// \todo Critical: this needs to be smarter, memory can expand quickly + /// in tight loops involving function calls + m_stack_holder->call_params.clear(); + } + } private: /** @@ -1083,6 +1106,7 @@ namespace chaiscript struct Stack_Holder { Stack_Holder() + : call_depth(0) { Stack s(new StackData()); s->push_back(Scope()); @@ -1090,6 +1114,9 @@ namespace chaiscript } std::deque stacks; + + std::list call_params; + int call_depth; }; std::vector m_conversions; diff --git a/include/chaiscript/language/chaiscript_common.hpp b/include/chaiscript/language/chaiscript_common.hpp index ea4b747..8428c38 100644 --- a/include/chaiscript/language/chaiscript_common.hpp +++ b/include/chaiscript/language/chaiscript_common.hpp @@ -214,6 +214,34 @@ namespace chaiscript chaiscript::detail::Dispatch_Engine &m_de; }; + /// Creates a new functon call and pops it on destruction + struct Function_Push_Pop + { + Function_Push_Pop(chaiscript::detail::Dispatch_Engine &t_de) + : m_de(t_de) + { + m_de.new_function_call(); + } + + ~Function_Push_Pop() + { + m_de.pop_function_call(); + } + + void save_params(const std::vector &t_params) + { + m_de.save_function_params(t_params); + } + + + private: + // explicitly unimplemented copy and assignment + Function_Push_Pop(const Function_Push_Pop &); + Function_Push_Pop& operator=(const Function_Push_Pop &); + + chaiscript::detail::Dispatch_Engine &m_de; + }; + /// Creates a new scope then pops it on destruction struct Stack_Push_Pop { @@ -231,8 +259,8 @@ namespace chaiscript private: // explicitly unimplemented copy and assignment - Stack_Push_Pop(const Scope_Push_Pop &); - Stack_Push_Pop& operator=(const Scope_Push_Pop &); + Stack_Push_Pop(const Stack_Push_Pop &); + Stack_Push_Pop& operator=(const Stack_Push_Pop &); chaiscript::detail::Dispatch_Engine &m_de; }; diff --git a/include/chaiscript/language/chaiscript_eval.hpp b/include/chaiscript/language/chaiscript_eval.hpp index 66e214f..70734fb 100644 --- a/include/chaiscript/language/chaiscript_eval.hpp +++ b/include/chaiscript/language/chaiscript_eval.hpp @@ -182,6 +182,7 @@ namespace chaiscript AST_Node(t_ast_node_text, t_id, t_fname, t_start_line, t_start_col, t_end_line, t_end_col) { } virtual ~Fun_Call_AST_Node() {} virtual Boxed_Value eval_internal(chaiscript::detail::Dispatch_Engine &t_ss){ + chaiscript::eval::detail::Function_Push_Pop fpp(t_ss); dispatch::Param_List_Builder plb; if ((this->children.size() > 1) && (this->children[1]->identifier == AST_Node_Type::Arg_List)) { @@ -190,6 +191,8 @@ namespace chaiscript } } + fpp.save_params(plb.objects); + Boxed_Value fn = this->children[0]->eval(t_ss); try { @@ -431,6 +434,7 @@ namespace chaiscript if (this->children.size() > 1) { for (size_t i = 2; i < this->children.size(); i+=2) { + chaiscript::eval::detail::Function_Push_Pop fpp(t_ss); dispatch::Param_List_Builder plb; plb << retval; @@ -440,6 +444,8 @@ namespace chaiscript } } + fpp.save_params(plb.objects); + std::string fun_name; if ((this->children[i]->identifier == AST_Node_Type::Fun_Call) || (this->children[i]->identifier == AST_Node_Type::Array_Call)) { fun_name = this->children[i]->children[0]->text;