Mercurial > octave
changeset 28804:3719f5d452d4 stable
refactor implementation of END indexing in interpreter (bug #58953)
* oct-lvalue.h, oct-lvalue.cc (octave_lvalue::eval_for_numel): New function.
(octave_lvalue::m_nel): Delete data member.
(octave_lvalue::numel (octave_idx_type)): Delete.
(octave_lvalue::numel (void) const): Compute result instead of
returning cached value.
* pt-eval.h, pt-eval.cc (tree_evaluator::convert_to_const_vector):
Simplify.
(Fend): Call tree_evaluator::evaluate_end_expression to do the real work.
(tree_evaluator::make_value_list): Don't check for magic_end here.
(end_value, tree_evaluator::evaluate_end_expression): New functions.
(tree_evaluator::m_index_list, tree_evaluator::m_index_type): New data members.
(tree_evaluator::set_indexed_object, tree_evaluator::index_list,
tree_evaluator::set_index_list, tree_evaluator::clear_index_list,
tree_evaluator::append_index_list, tree_evaluator::index_type):
New functions.
* pt-idx.cc (make_value_list): Delete.
(tree_index_expression::lvalue, tree_index_expression::evaluate_n):
Refactor to avoid evaluating partial expressions to obtain values for
the END function. Instead, cache info necessary to do that job as
needed and inside the END function itself.
| author | John W. Eaton <jwe@octave.org> and Fernando Alvarruiz |
|---|---|
| date | Mon, 21 Sep 2020 10:45:18 -0400 |
| parents | 7e0712b3388f |
| children | 2219b0ed8754 |
| files | libinterp/parse-tree/oct-lvalue.cc libinterp/parse-tree/oct-lvalue.h libinterp/parse-tree/oct-parse.yy libinterp/parse-tree/pt-eval.cc libinterp/parse-tree/pt-eval.h libinterp/parse-tree/pt-idx.cc test/bug-58593/bug-58593.tst test/bug-58593/module.mk test/bug-58593/myclass1.m test/bug-58593/myclass2.m test/module.mk test/struct.tst |
| diffstat | 12 files changed, 598 insertions(+), 343 deletions(-) [+] |
line wrap: on
line diff
--- a/libinterp/parse-tree/oct-lvalue.cc Sun Sep 27 12:24:24 2020 -0400 +++ b/libinterp/parse-tree/oct-lvalue.cc Mon Sep 21 10:45:18 2020 -0400 @@ -28,6 +28,7 @@ #endif #include "error.h" +#include "errwarn.h" #include "ovl.h" #include "oct-lvalue.h" #include "ov.h" @@ -56,6 +57,115 @@ m_frame->assign (op, m_sym, m_type, m_idx, rhs); } + octave_idx_type octave_lvalue::numel (void) const + { + // Return 1 if there is no index because without an index there + // should be no way to have a cs-list here. Cs-lists may be passed + // around internally but they are not supposed to be stored as + // single symbols in a stack frame. + + size_t num_indices = m_idx.size (); + + if (num_indices == 0) + return 1; + + switch (m_type[num_indices-1]) + { + case '(': + return 1; + + case '{': + { + // FIXME: Duplicate code in '.' case below... + + // Evaluate, skipping the last index. + + std::string tmp_type = m_type; + std::list<octave_value_list> tmp_idx = m_idx; + + tmp_type.pop_back (); + tmp_idx.pop_back (); + + octave_value tmp = eval_for_numel (tmp_type, tmp_idx); + + octave_value_list tidx = m_idx.back (); + + if (tmp.is_undefined ()) + { + if (tidx.has_magic_colon ()) + err_invalid_inquiry_subscript (); + + tmp = Cell (); + } + else if (tmp.is_zero_by_zero () + && (tmp.is_matrix_type () || tmp.is_string ())) + { + tmp = Cell (); + } + + return tmp.xnumel (tidx); + } + break; + + case '.': + { + // Evaluate, skipping either the last index or the last two + // indices if we are looking at "(idx).field". + + std::string tmp_type = m_type; + std::list<octave_value_list> tmp_idx = m_idx; + + tmp_type.pop_back (); + tmp_idx.pop_back (); + + bool paren_dot = num_indices > 1 && m_type[num_indices-2] == '('; + + // Index for paren operator, if any. + octave_value_list pidx; + + if (paren_dot) + { + pidx = tmp_idx.back (); + + tmp_type.pop_back (); + tmp_idx.pop_back (); + } + + octave_value tmp = eval_for_numel (tmp_type, tmp_idx); + + bool autoconv = (tmp.is_zero_by_zero () + && (tmp.is_matrix_type () || tmp.is_string () + || tmp.iscell ())); + + if (paren_dot) + { + // Use octave_map, not octave_scalar_map so that the + // dimensions are 0x0, not 1x1. + + if (tmp.is_undefined ()) + { + if (pidx.has_magic_colon ()) + err_invalid_inquiry_subscript (); + + tmp = octave_map (); + } + else if (autoconv) + tmp = octave_map (); + + return tmp.xnumel (pidx); + } + else if (tmp.is_undefined () || autoconv) + return 1; + else + return tmp.xnumel (octave_value_list ()); + } + break; + + default: + panic_impossible (); + } + } + void octave_lvalue::set_index (const std::string& t, const std::list<octave_value_list>& i) { @@ -105,4 +215,29 @@ return (is_black_hole () ? octave_value () : m_frame->value (m_sym, m_type, m_idx)); } + + octave_value + octave_lvalue::eval_for_numel (const std::string& type, + const std::list<octave_value_list>& idx) const + { + octave_value retval; + + try + { + retval = m_frame->varval (m_sym); + + if (retval.is_constant () && ! idx.empty ()) + retval = retval.subsref (type, idx); + } + catch (const execution_exception&) + { + // Ignore an error and treat it as undefined. The error + // could happen because there is an index is out of range + // and we will be resizing a cell array. + + retval = octave_value (); + } + + return retval; + } }
--- a/libinterp/parse-tree/oct-lvalue.h Sun Sep 27 12:24:24 2020 -0400 +++ b/libinterp/parse-tree/oct-lvalue.h Mon Sep 21 10:45:18 2020 -0400 @@ -43,7 +43,7 @@ octave_lvalue (const symbol_record& sr, const std::shared_ptr<stack_frame>& frame) : m_sym (sr), m_frame (frame), m_black_hole (false), - m_type (), m_idx (), m_nel (1) + m_type (), m_idx () { } octave_lvalue (const octave_lvalue&) = default; @@ -66,9 +66,7 @@ void assign (octave_value::assign_op, const octave_value&); - void numel (octave_idx_type n) { m_nel = n; } - - octave_idx_type numel (void) const { return m_nel; } + octave_idx_type numel (void) const; void set_index (const std::string& t, const std::list<octave_value_list>& i); @@ -86,6 +84,10 @@ private: + octave_value + eval_for_numel (const std::string& type, + const std::list<octave_value_list>& idx) const; + symbol_record m_sym; std::shared_ptr<stack_frame> m_frame;
--- a/libinterp/parse-tree/oct-parse.yy Sun Sep 27 12:24:24 2020 -0400 +++ b/libinterp/parse-tree/oct-parse.yy Mon Sep 21 10:45:18 2020 -0400 @@ -595,6 +595,10 @@ // tree_argument_list objects can't be empty or have leading or trailing // commas, but those are all allowed in matrix and cell array rows. +// FIXME: is tree_argument_list the best object for this purpose, or +// should we have a separate one intended specifically to represent the +// list of objects that make up elements in cell and matrix expressions? + cell_or_matrix_row : // empty { $$ = nullptr; }
--- a/libinterp/parse-tree/pt-eval.cc Sun Sep 27 12:24:24 2020 -0400 +++ b/libinterp/parse-tree/pt-eval.cc Mon Sep 21 10:45:18 2020 -0400 @@ -36,6 +36,7 @@ #include "cmd-edit.h" #include "file-ops.h" #include "file-stat.h" +#include "lo-array-errwarn.h" #include "lo-ieee.h" #include "oct-env.h" @@ -1635,7 +1636,7 @@ } // END is documented in op-kw-docs. -DEFCONSTMETHOD (end, interp, , , +DEFCONSTMETHOD (end, interp, args, , doc: /* -*- texinfo -*- @deftypefn {} {} end Last element of an array or the end of any @code{for}, @code{parfor}, @@ -1661,68 +1662,9 @@ @seealso{for, parfor, if, do, while, function, switch, try, unwind_protect} @end deftypefn */) { - octave_value retval; - octave::tree_evaluator& tw = interp.get_evaluator (); - octave_value indexed_object = tw.indexed_object (); - int index_position = tw.index_position (); - int num_indices = tw.num_indices (); - - // If indexed_object is nullptr, then this use of 'end' is either - // appearing in a function call argument list or in an attempt to - // index an undefined symbol. There seems to be no reasonable way to - // provide a better error message. So just fail with an invalid use - // message. See bug #58830. - - if (indexed_object.is_undefined ()) - error ("invalid use of 'end': may only be used to index existing value"); - - if (indexed_object.isobject ()) - { - octave_value_list args; - - args(2) = num_indices; - args(1) = index_position + 1; - args(0) = indexed_object; - - std::string class_name = indexed_object.class_name (); - - octave::symbol_table& symtab = interp.get_symbol_table (); - - octave_value meth = symtab.find_method ("end", class_name); - - if (meth.is_defined ()) - return octave::feval (meth.function_value (), args, 1); - } - - dim_vector dv = indexed_object.dims (); - int ndims = dv.ndims (); - - if (num_indices < ndims) - { - for (int i = num_indices; i < ndims; i++) - dv(num_indices-1) *= dv(i); - - if (num_indices == 1) - { - ndims = 2; - dv.resize (ndims); - dv(1) = 1; - } - else - { - ndims = num_indices; - dv.resize (ndims); - } - } - - if (index_position < ndims) - retval = dv(index_position); - else - retval = 1; - - return retval; + return tw.evaluate_end_expression (args); } /* @@ -1740,59 +1682,31 @@ namespace octave { octave_value_list - tree_evaluator::convert_to_const_vector (tree_argument_list *arg_list, - const octave_value& object) + tree_evaluator::convert_to_const_vector (tree_argument_list *args) { - // END doesn't make sense as a direct argument for a function (i.e., - // "fcn (end)" is invalid but "fcn (array (end))" is OK). Maybe we - // need a different way of asking an octave_value object this - // question? - - bool stash_object - = (object.is_defined () - && ! (object.is_function () || object.is_function_handle ())); - - unwind_protect_var<octave_value> upv1 (m_indexed_object); - unwind_protect_var<int> upv2 (m_index_position); - unwind_protect_var<int> upv3 (m_num_indices); - - if (stash_object) - m_indexed_object = object; - - int len = arg_list->length (); - - std::list<octave_value> args; - - auto p = arg_list->begin (); - for (int k = 0; k < len; k++) + std::list<octave_value> arg_vals; + + for (auto elt : *args) { - if (stash_object) - { - m_index_position = k; - m_num_indices = len; - } - - tree_expression *elt = *p++; - - if (elt) + // FIXME: is it possible for elt to be invalid? + + if (! elt) + break; + + octave_value tmp = elt->evaluate (*this); + + if (tmp.is_cs_list ()) { - octave_value tmp = elt->evaluate (*this); - - if (tmp.is_cs_list ()) - { - octave_value_list tmp_ovl = tmp.list_value (); - - for (octave_idx_type i = 0; i < tmp_ovl.length (); i++) - args.push_back (tmp_ovl(i)); - } - else if (tmp.is_defined ()) - args.push_back (tmp); + octave_value_list tmp_ovl = tmp.list_value (); + + for (octave_idx_type i = 0; i < tmp_ovl.length (); i++) + arg_vals.push_back (tmp_ovl(i)); } - else - break; + else if (tmp.is_defined ()) + arg_vals.push_back (tmp); } - return octave_value_list (args); + return octave_value_list (arg_vals); } octave_value_list @@ -3863,8 +3777,7 @@ octave_value_list tree_evaluator::make_value_list (tree_argument_list *args, - const string_vector& arg_nm, - const octave_value& object, bool rvalue) + const string_vector& arg_nm) { octave_value_list retval; @@ -3873,10 +3786,40 @@ unwind_protect_var<const std::list<octave_lvalue> *> upv (m_lvalue_list, nullptr); - if (rvalue && args->has_magic_end () && object.is_undefined ()) - err_invalid_inquiry_subscript (); - - retval = convert_to_const_vector (args, object); + int len = args->length (); + + unwind_protect_var<int> upv2 (m_index_position); + unwind_protect_var<int> upv3 (m_num_indices); + + m_num_indices = len; + + std::list<octave_value> arg_vals; + + int k = 0; + + for (auto elt : *args) + { + // FIXME: is it possible for elt to be invalid? + + if (! elt) + break; + + m_index_position = k++; + + octave_value tmp = elt->evaluate (*this); + + if (tmp.is_cs_list ()) + { + octave_value_list tmp_ovl = tmp.list_value (); + + for (octave_idx_type i = 0; i < tmp_ovl.length (); i++) + arg_vals.push_back (tmp_ovl(i)); + } + else if (tmp.is_defined ()) + arg_vals.push_back (tmp); + } + + retval = octave_value_list (arg_vals); } octave_idx_type n = retval.length (); @@ -4121,6 +4064,132 @@ m_debugger_stack.top()->dbquit (all); } + static octave_value end_value (const octave_value& value, + octave_idx_type index_position, + octave_idx_type num_indices) + { + dim_vector dv = value.dims (); + int ndims = dv.ndims (); + + if (num_indices < ndims) + { + for (int i = num_indices; i < ndims; i++) + dv(num_indices-1) *= dv(i); + + if (num_indices == 1) + { + ndims = 2; + dv.resize (ndims); + dv(1) = 1; + } + else + { + ndims = num_indices; + dv.resize (ndims); + } + } + + return (index_position < ndims + ? octave_value (dv(index_position)) : octave_value (1.0)); + } + + octave_value_list + tree_evaluator::evaluate_end_expression (const octave_value_list& args) + { + int nargin = args.length (); + + if (nargin != 0 && nargin != 3) + print_usage (); + + if (nargin == 3) + { + octave_idx_type index_position + = args(1).xidx_type_value ("end: K must be integer value"); + + if (index_position < 1) + error ("end: K must be greater than zero"); + + octave_idx_type num_indices + = args(2).xidx_type_value ("end: N must be integer value"); + + if (num_indices < 1) + error ("end: N must be greater than zero"); + + return end_value (args(0), index_position-1, num_indices); + } + + // If m_indexed_object is undefined, then this use of 'end' is + // either appearing in a function call argument list or in an + // attempt to index an undefined symbol. There seems to be no + // reasonable way to provide a better error message. So just fail + // with an invalid use message. See bug #58830. + + if (m_indexed_object.is_undefined ()) + error ("invalid use of 'end': may only be used to index existing value"); + + octave_value expr_result; + + if (m_index_list.empty ()) + expr_result = m_indexed_object; + else + { + try + { + // When evaluating "end" with no arguments, we should have + // been called from the built-in Fend function that appears + // in the context of an argument list. Fend will be + // evaluated in its own stack frame. But we need to + // evaluate the partial expression that the special "end" + // token applies to in the calling stack frame. + + unwind_action act ([this] (size_t frm) + { + m_call_stack.restore_frame (frm); + }, m_call_stack.current_frame ()); + + size_t n = m_call_stack.find_current_user_frame (); + m_call_stack.goto_frame (n); + + // End is only valid inside argument lists used for + // indexing. The dispatch class is set by the function that + // evaluates the argument list. + + // Silently ignore extra output values. + + octave_value_list tmp + = m_indexed_object.subsref (m_index_type, m_index_list, 1); + + expr_result = tmp.length () ? tmp(0) : octave_value (); + + if (expr_result.is_cs_list ()) + err_indexed_cs_list (); + } + catch (index_exception&) + { + error ("error evaluating partial expression for END"); + } + } + + if (expr_result.isobject ()) + { + // FIXME: is there a better way to lookup and execute a method + // that handles all the details like setting the dispatch class + // appropriately? + + std::string dispatch_class = expr_result.class_name (); + + symbol_table& symtab = m_interpreter.get_symbol_table (); + + octave_value meth = symtab.find_method ("end", dispatch_class); + + if (meth.is_defined ()) + return m_interpreter.feval + (meth, ovl (expr_result, m_index_position+1, m_num_indices), 1); + } + + return end_value (expr_result, m_index_position, m_num_indices); + } + octave_value tree_evaluator::PS4 (const octave_value_list& args, int nargout) {
--- a/libinterp/parse-tree/pt-eval.h Sun Sep 27 12:24:24 2020 -0400 +++ b/libinterp/parse-tree/pt-eval.h Mon Sep 21 10:45:18 2020 -0400 @@ -138,8 +138,8 @@ m_echo_state (false), m_echo_file_name (), m_echo_file_pos (1), m_echo_files (), m_in_loop_command (false), m_breaking (0), m_continuing (0), m_returning (0), - m_indexed_object (), m_index_position (0), - m_num_indices (0) + m_indexed_object (), m_index_list (), m_index_type (), + m_index_position (0), m_num_indices (0) { } // No copying! @@ -355,9 +355,7 @@ void undefine_parameter_list (tree_parameter_list *param_list); - octave_value_list - convert_to_const_vector (tree_argument_list *arg_list, - const octave_value& object = octave_value ()); + octave_value_list convert_to_const_vector (tree_argument_list *arg_list); octave_value_list convert_return_list_to_const_vector @@ -632,10 +630,46 @@ return m_indexed_object; } + void set_indexed_object (const octave_value& obj = octave_value ()) + { + m_indexed_object = obj; + } + + const std::list<octave_value_list>& index_list (void) const + { + return m_index_list; + } + + void set_index_list (const std::string& index_type, + const std::list<octave_value_list>& index_list) + { + m_index_type = index_type; + m_index_list = index_list; + } + + void clear_index_list (void) + { + m_index_type = ""; + m_index_list.clear (); + } + + void append_index_list (char type, const octave_value_list& idx) + { + m_index_type += type; + m_index_list.push_back (idx); + } + + const std::string& index_type (void) const + { + return m_index_type; + } + int index_position (void) const { return m_index_position; } int num_indices (void) const { return m_num_indices; } + octave_value_list evaluate_end_expression (const octave_value_list& args); + const std::list<octave_lvalue> * lvalue_list (void) const { return m_lvalue_list; @@ -704,9 +738,7 @@ bool return_list, bool verbose = false); octave_value_list - make_value_list (tree_argument_list *args, - const string_vector& arg_nm, - const octave_value& object, bool rvalue = true); + make_value_list (tree_argument_list *args, const string_vector& arg_nm); std::list<octave_lvalue> make_lvalue_list (tree_argument_list *); @@ -831,8 +863,11 @@ // Nonzero means we're returning from a function. int m_returning; - // Used by END function. + // The following are all used by the END function. Maybe they + // should be kept together in a separate object? octave_value m_indexed_object; + std::list<octave_value_list> m_index_list; + std::string m_index_type; int m_index_position; int m_num_indices; };
--- a/libinterp/parse-tree/pt-idx.cc Sun Sep 27 12:24:24 2020 -0400 +++ b/libinterp/parse-tree/pt-idx.cc Mon Sep 21 10:45:18 2020 -0400 @@ -147,40 +147,6 @@ return m_expr->name (); } - static inline octave_value_list - make_value_list (tree_evaluator& tw, - tree_argument_list *m_args, - const string_vector& m_arg_nm, const octave_value& object, - bool rvalue = true) - { - // FIXME: This function duplicates tree_evaluator::make_value_list. - // See also the way convert_to_const_vector is used in - // tree_index_expression::evaluate_n. - - octave_value_list retval; - - if (m_args) - { - unwind_action act ([&tw] (const std::list<octave_lvalue> *lvl) - { - tw.set_lvalue_list (lvl); - }, tw.lvalue_list ()); - tw.set_lvalue_list (nullptr); - - if (rvalue && m_args->has_magic_end () && object.is_undefined ()) - err_invalid_inquiry_subscript (); - - retval = tw.convert_to_const_vector (m_args, object); - } - - octave_idx_type n = retval.length (); - - if (n > 0) - retval.stash_name_tags (m_arg_nm); - - return retval; - } - std::string tree_index_expression::get_struct_index (tree_evaluator& tw, @@ -210,7 +176,6 @@ tree_index_expression::lvalue (tree_evaluator& tw) { std::list<octave_value_list> idx; - std::string tmp_type; int n = m_args.size (); @@ -220,73 +185,37 @@ octave_lvalue retval = m_expr->lvalue (tw); - octave_value tmp = retval.value (); + unwind_action + act ([&tw] (const octave_value& val, + const std::string& index_type, + const std::list<octave_value_list>& index_list) + { + tw.set_indexed_object (val); + tw.set_index_list (index_type, index_list); + }, tw.indexed_object (), tw.index_type (), tw.index_list ()); - octave_idx_type tmpi = 0; - std::list<octave_value_list> tmpidx; + tw.set_indexed_object (retval.value ()); + tw.clear_index_list (); for (int i = 0; i < n; i++) { - if (retval.numel () != 1) - err_indexed_cs_list (); - - if (tmpi < i) - { - try - { - tmp = tmp.subsref (m_type.substr (tmpi, i-tmpi), tmpidx, true); - } - catch (index_exception& e) // problems with range, invalid type etc. - { - tw.final_index_error (e, m_expr); - } - - tmpidx.clear (); - } - switch (m_type[i]) { case '(': { - octave_value_list tidx - = make_value_list (tw, *p_args, *p_arg_nm, tmp, false); - - idx.push_back (tidx); + octave_value_list tidx = tw.make_value_list (*p_args, *p_arg_nm); - if (i < n - 1) - { - if (m_type[i+1] != '.') - error ("() must be followed by . or close the index chain"); - - tmpidx.push_back (tidx); - tmpi = i+1; - } + tw.append_index_list ('(', tidx); + idx.push_back (tidx); } break; case '{': { - octave_value_list tidx - = make_value_list (tw, *p_args, *p_arg_nm, tmp, false); - - if (tmp.is_undefined ()) - { - if (tidx.has_magic_colon ()) - err_invalid_inquiry_subscript (); + octave_value_list tidx = tw.make_value_list (*p_args, *p_arg_nm); - tmp = Cell (); - } - else if (tmp.is_zero_by_zero () - && (tmp.is_matrix_type () || tmp.is_string ())) - { - tmp = Cell (); - } - - retval.numel (tmp.xnumel (tidx)); - + tw.append_index_list ('{', tidx); idx.push_back (tidx); - tmpidx.push_back (tidx); - tmpi = i; } break; @@ -294,47 +223,7 @@ { octave_value tidx = get_struct_index (tw, p_arg_nm, p_dyn_field); - bool autoconv = (tmp.is_zero_by_zero () - && (tmp.is_matrix_type () || tmp.is_string () - || tmp.iscell ())); - - if (i > 0 && m_type[i-1] == '(') - { - octave_value_list pidx = idx.back (); - - // Use octave_map, not octave_scalar_map so that the - // dimensions are 0x0, not 1x1. - if (tmp.is_undefined ()) - { - if (pidx.has_magic_colon ()) - err_invalid_inquiry_subscript (); - - tmp = octave_map (); - } - else if (autoconv) - tmp = octave_map (); - - retval.numel (tmp.xnumel (pidx)); - - tmpi = i-1; - tmpidx.push_back (tidx); - } - else - { - if (tmp.is_undefined () || autoconv) - { - tmpi = i+1; - tmp = octave_value (); - } - else - { - retval.numel (tmp.xnumel (octave_value_list ())); - - tmpi = i; - tmpidx.push_back (tidx); - } - } - + tw.append_index_list ('.', tidx); idx.push_back (tidx); } break; @@ -573,102 +462,127 @@ // files in a +packagename folder, they will also be an // octave_classdef_meta object, but we don't want to index them. - bool indexing_object = (base_expr_val.isobject () - || base_expr_val.isjava () - || (base_expr_val.is_classdef_meta () - && ! base_expr_val.is_package ())); - std::list<octave_value_list> idx_list; - octave_value partial_expr_val = base_expr_val; - - for (int i = beg; i < n; i++) - { - if (i > beg) - { - tree_argument_list *al = *p_args; + { + // Note: need new scope so that the following unwind action will + // happen before we perform the final indexing for objects (for + // example). - if (! indexing_object || (al && al->has_magic_end ())) - { - // Evaluate what we have so far to find the value to - // pass to the END function. + unwind_action + act ([&tw] (const octave_value& val, + const std::string& index_type, + const std::list<octave_value_list>& index_list) + { + tw.set_indexed_object (val); + tw.set_index_list (index_type, index_list); + }, + tw.indexed_object (), + tw.index_type (), tw.index_list ()); - try - { - // Silently ignore extra output values. + tw.set_indexed_object (); + tw.clear_index_list (); - octave_value_list tmp_list - = base_expr_val.subsref (m_type.substr (beg, i-beg), - idx_list, nargout); + bool indexing_object = (base_expr_val.isobject () + || base_expr_val.isjava () + || (base_expr_val.is_classdef_meta () + && ! base_expr_val.is_package ())); + + octave_value partial_expr_val = base_expr_val; - partial_expr_val - = tmp_list.length () ? tmp_list(0) : octave_value (); + for (int i = beg; i < n; i++) + { + if (i > beg) + { + if (! indexing_object) + { + // Evaluate what we have so far. - if (! indexing_object) - { - base_expr_val = partial_expr_val; - - if (partial_expr_val.is_cs_list ()) - err_indexed_cs_list (); - - retval = partial_expr_val; + try + { + // Silently ignore extra output values. - beg = i; - idx_list.clear (); + octave_value_list tmp_list + = base_expr_val.subsref (m_type.substr (beg, i-beg), + idx_list, nargout); + + partial_expr_val + = tmp_list.length () ? tmp_list(0) : octave_value (); + + base_expr_val = partial_expr_val; - if (partial_expr_val.isobject () - || partial_expr_val.isjava () - || (partial_expr_val.is_classdef_meta () - && ! partial_expr_val.is_package ())) - { - // Found an object, so now we'll build up - // complete index list for one big subsref - // call from this point on. + if (partial_expr_val.is_cs_list ()) + err_indexed_cs_list (); + + retval = partial_expr_val; + + beg = i; + idx_list.clear (); + tw.clear_index_list (); - // FIXME: is is also possible to have a - // static method call buried somewhere in - // the depths of a complex indexing - // expression so that we would also need to - // check for an octave_classdef_meta object - // here? + if (partial_expr_val.isobject () + || partial_expr_val.isjava () + || (partial_expr_val.is_classdef_meta () + && ! partial_expr_val.is_package ())) + { + // Found an object, so now we'll build up + // complete index list for one big subsref + // call from this point on. - indexing_object = true; - } - } - } - catch (index_exception& e) - { - tw.final_index_error (e, m_expr); - } - } - } + // FIXME: is is also possible to have a + // static method call buried somewhere in + // the depths of a complex indexing + // expression so that we would also need to + // check for an octave_classdef_meta object + // here? + + indexing_object = true; + } + } + catch (index_exception& e) + { + tw.final_index_error (e, m_expr); + } + } + } + + tw.set_indexed_object (partial_expr_val); - switch (m_type[i]) - { - case '(': - idx_list.push_back (make_value_list (tw, *p_args, *p_arg_nm, - partial_expr_val)); - break; + switch (m_type[i]) + { + case '(': + { + octave_value_list tmp = tw.make_value_list (*p_args, *p_arg_nm); + tw.append_index_list ('(', tmp); + idx_list.push_back (tmp); + } + break; - case '{': - idx_list.push_back (make_value_list (tw, *p_args, *p_arg_nm, - partial_expr_val)); - break; + case '{': + { + octave_value_list tmp = tw.make_value_list (*p_args, *p_arg_nm); + tw.append_index_list ('{', tmp); + idx_list.push_back (tmp); + } + break; - case '.': - idx_list.push_back (octave_value - (get_struct_index (tw, p_arg_nm, p_dyn_field))); - break; + case '.': + { + octave_value tmp = get_struct_index (tw, p_arg_nm, p_dyn_field); + tw.append_index_list ('.', tmp); + idx_list.push_back (tmp); + } + break; - default: - panic_impossible (); - } + default: + panic_impossible (); + } - p_args++; - p_arg_nm++; - p_dyn_field++; - } - + p_args++; + p_arg_nm++; + p_dyn_field++; + } + } // If ! idx_list.empty () that means we still have stuff to index // otherwise they would have been dealt with and idx_list would have @@ -760,16 +674,11 @@ } } - // Delete any temporary values prior to pushing the result and - // returning so that destructors for any temporary classdef handle - // objects will be called before we return. Otherwise, the - // destructor may push result values that will wipe out the result - // that we push below. Although the method name is "push_result" - // there is only a single register (either an octave_value or an - // octave_value_list) not a stack. + // Delete any temporary values prior to returning so that + // destructors for any temporary classdef handle objects will be + // called before we return. idx_list.clear (); - partial_expr_val = octave_value (); base_expr_val = octave_value (); val = octave_value ();
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/bug-58593/bug-58593.tst Mon Sep 21 10:45:18 2020 -0400 @@ -0,0 +1,40 @@ +######################################################################## +## +## Copyright (C) 2017-2020 The Octave Project Developers +## +## See the file COPYRIGHT.md in the top-level directory of this +## distribution or <https://octave.org/copyright/>. +## +## This file is part of Octave. +## +## Octave is free software: you can redistribute it and/or modify it +## under the terms of the GNU General Public License as published by +## the Free Software Foundation, either version 3 of the License, or +## (at your option) any later version. +## +## Octave is distributed in the hope that it will be useful, but +## WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## +## You should have received a copy of the GNU General Public License +## along with Octave; see the file COPYING. If not, see +## <https://www.gnu.org/licenses/>. +## +######################################################################## + +%!test <*58593> +%! obj = myclass1 (); +%! assert (obj.data (plus (minus (end,1), 1)), 1005) + +%!test <*58593> +%! obj = myclass2 (); +%! assert (obj.alldata, 1001:1005) + +%!test <*58593> +%! obj = myclass2 (); +%! assert (obj(end), 1004) + +%!test <*58593> +%! obj = myclass2 (); +%! assert (obj.data(end), 1005)
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/bug-58593/module.mk Mon Sep 21 10:45:18 2020 -0400 @@ -0,0 +1,6 @@ +bug_58593_TEST_FILES = \ + %reldir%/bug-58593.tst \ + %reldir%/myclass1.m \ + %reldir%/myclass2.m + +TEST_FILES += $(bug_58593_TEST_FILES)
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/bug-58593/myclass1.m Mon Sep 21 10:45:18 2020 -0400 @@ -0,0 +1,10 @@ +classdef myclass1 < handle + properties + data + end + methods + function obj = myclass1 () + obj.data = 1001:1005; + end + end +end
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/bug-58593/myclass2.m Mon Sep 21 10:45:18 2020 -0400 @@ -0,0 +1,40 @@ +classdef myclass2 < handle + properties + data_ + end + + methods + function obj = myclass2(data) + obj.data_ = 1001:1005; + end + + function r = subsref(obj,S) + switch S(1).type + case '.' + switch S(1).subs + case 'data' + % Transform: obj.data --> obj.data_ + r = subsref (obj.data_, S(2:end)); + case 'alldata' + % Transform: obj.data --> obj.data_(1:end) + % This expression should trigger *builtin* subsref *twice* (one + % for the evaluation of 'end', and the other for the whole rvalue). + % 'end' here is also builtin 'end' + r = obj.data_(1:end); + otherwise + error('Incorrect usage') + end + case '()' + % Transform: obj(index) --> obj.data_(1:end) + r = subsref(obj.data_,S); + otherwise + error('Incorrect usage') + end + end + + function r = end(obj,k,n) + % We subtract 1 from the "real" end of obj.data_ + r = builtin('end',obj.data_,k,n)-1; + end + end +end
--- a/test/module.mk Sun Sep 27 12:24:24 2020 -0400 +++ b/test/module.mk Mon Sep 21 10:45:18 2020 -0400 @@ -84,6 +84,7 @@ include %reldir%/bug-54995/module.mk include %reldir%/bug-55758/module.mk include %reldir%/bug-58572/module.mk +include %reldir%/bug-58593/module.mk include %reldir%/class-concat/module.mk include %reldir%/classdef/module.mk include %reldir%/classdef-multiple-inheritance/module.mk
--- a/test/struct.tst Sun Sep 27 12:24:24 2020 -0400 +++ b/test/struct.tst Mon Sep 21 10:45:18 2020 -0400 @@ -155,9 +155,13 @@ %! assert ({b.name}, {1, 2, "b", "e", 3, 4}); %! assert ({b.value}, {100, "5", 100, "6", 100, "7"}); +%!error <deal: nargin> +%! [a(1:3).x] = deal ([1, 5], [3, 7], [8, 9]); +%! [a(2:3).x(2)] = deal (10, 11); + %!error <a cs-list cannot be further indexed> %! [a(1:3).x] = deal ([1, 5], [3, 7], [8, 9]); -%! [a(2:3).x(2)] = deal (10, 11); +%! [a(2:3).x(2)] = 1; %!error <a cs-list cannot be further indexed> %! [a(1:3).x] = deal ([1, 5], [3, 7], [8, 9]);