# HG changeset patch # User Jaroslav Hajek # Date 1232482517 -3600 # Node ID 3d8a914c580e99d0294568ce00e0bbdb8f52fc88 # Parent faccdb98d9539dcc687dce0a3c239dd3bed2f3af improve parser indexed assigment code diff --git a/src/ChangeLog b/src/ChangeLog --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,15 @@ +2009-01-19 Jaroslav Hajek + + * ov.h (octave_value::make_unique (int)): New method. + * oct-obj.h (octave_value_list::octave_value_list (const Cell&)): New + constructor. + * ov-cell.cc (octave_cell::subsasgn): Allow composed {} indexing + in multi-assignments. Optimize. + * ov-struct.cc (octave_struct::subsasgn): Correct composed {} indexing + in multi-assignments. Optimize & fix bugs. + * pt-idx.cc (tree_index_expression::lvalue): Rewrite to allow + specifying cs-list anywhere in index chain, be more robust. + 2009-01-19 John W. Eaton * lex.l (grab_comment_block): If not reading input from a file, diff --git a/src/oct-obj.cc b/src/oct-obj.cc --- a/src/oct-obj.cc +++ b/src/oct-obj.cc @@ -27,6 +27,14 @@ #include "error.h" #include "oct-obj.h" +#include "Cell.h" + +octave_value_list::octave_value_list (const Cell& tc) + : data (tc.numel ()) +{ + for (octave_idx_type i = 0; i < tc.numel (); i++) + data[i] = tc(i); +} octave_allocator octave_value_list::allocator (sizeof (octave_value_list)); diff --git a/src/oct-obj.h b/src/oct-obj.h --- a/src/oct-obj.h +++ b/src/oct-obj.h @@ -31,6 +31,7 @@ #include "str-vec.h" #include "ov.h" +class Cell; class OCTINTERP_API @@ -47,6 +48,8 @@ octave_value_list (const octave_value& tc) : data (1, tc) { } + octave_value_list (const Cell& tc); + octave_value_list (const octave_value_list& obj) : data (obj.data), names (obj.names) { } diff --git a/src/ov-cell.cc b/src/ov-cell.cc --- a/src/ov-cell.cc +++ b/src/ov-cell.cc @@ -90,19 +90,7 @@ if (tcell.length () == 1) retval(0) = tcell(0,0); else - { - octave_idx_type n = tcell.numel (); - - octave_value_list lst (n, octave_value ()); - - for (octave_idx_type i = 0; i < n; i++) - { - OCTAVE_QUIT; - lst(i) = tcell(i); - } - - retval(0) = octave_value (lst, true); - } + retval = octave_value (octave_value_list (tcell), true); } } break; @@ -179,36 +167,59 @@ case '{': { - octave_value tmp = do_index_op (idx.front (), true); + Cell tmpc = matrix.index (idx.front (), true); if (! error_state) { - if (tmp.numel () == 1) - { - tmp = tmp.cell_value ()(0,0); + std::list next_idx (idx); + + next_idx.erase (next_idx.begin ()); + + std::string next_type = type.substr (1); - std::list next_idx (idx); + if (rhs.is_cs_list ()) + { + const octave_value_list rhsl = rhs.list_value (); + if (tmpc.numel () == rhsl.length ()) + { + for (octave_idx_type k = 0; k < tmpc.numel () && ! error_state; k++) + { + octave_value tmp = tmpc (k); + if (! tmp.is_defined () || tmp.is_zero_by_zero ()) + { + tmp = octave_value::empty_conv (next_type, rhs); + tmp.make_unique (); // probably a no-op. + } + else + // optimization: ignore the copy still stored inside our array and in tmpc. + tmp.make_unique (2); - next_idx.erase (next_idx.begin ()); + tmpc(k) = tmp.subsasgn (next_type, next_idx, rhsl(k)); + } + + t_rhs = octave_value (octave_value_list (tmpc), true); + } + else + error ("invalid cs-list length in assignment"); + } + else if (tmpc.numel () == 1) + { + octave_value tmp = tmpc(0); if (! tmp.is_defined () || tmp.is_zero_by_zero ()) - tmp = octave_value::empty_conv (type.substr (1), rhs); + { + tmp = octave_value::empty_conv (type.substr (1), rhs); + tmp.make_unique (); // probably a no-op. + } else - { - // This is a bit of black magic. tmp is a shallow copy - // of an element inside this cell, and maybe more. To - // prevent make_unique from always forcing a copy, we - // temporarily delete the stored value. - assign (idx.front (), octave_value ()); - tmp.make_unique (); - assign (idx.front (), Cell (tmp)); - } + // optimization: ignore the copy still stored inside our array and in tmpc. + tmp.make_unique (2); if (! error_state) - t_rhs = tmp.subsasgn (type.substr (1), next_idx, rhs); + t_rhs = tmp.subsasgn (next_type, next_idx, rhs); } else - error ("scalar indices required for {} in assignment."); + error ("invalid assignment to cs-list outside multiple assignment."); } } break; @@ -272,7 +283,7 @@ // Regularize a null matrix if stored into a cell. octave_base_matrix::assign (i, Cell (t_rhs.storable_value ())); else if (! error_state) - error ("scalar indices required for {} in assignment."); + error ("invalid assignment to cs-list outside multiple assignment."); if (! error_state) { diff --git a/src/ov-struct.cc b/src/ov-struct.cc --- a/src/ov-struct.cc +++ b/src/ov-struct.cc @@ -172,20 +172,15 @@ */ octave_value -octave_struct::numeric_conv (const Cell& val, +octave_struct::numeric_conv (const octave_value& val, const std::string& type) { octave_value retval; - if (val.length () == 1) - { - retval = val(0); - - if (type.length () > 0 && type[0] == '.' && ! retval.is_map ()) - retval = Octave_map (); - } + if (type.length () > 0 && type[0] == '.' && ! val.is_map ()) + retval = Octave_map (); else - gripe_invalid_index_for_assignment (); + retval = val; return retval; } @@ -201,6 +196,9 @@ octave_value t_rhs = rhs; + // This is handy for calling const methods of map. + const Octave_map& cmap = const_cast (map); + if (n > 1 && ! (type.length () == 2 && type[0] == '(' && type[1] == '.')) { switch (type[0]) @@ -218,45 +216,66 @@ std::string key = key_idx(0).string_value (); - octave_value u; + std::list next_idx (idx); + + // We handled two index elements, so subsasgn to + // needs to skip both of them. - if (! map.contains (key)) - u = octave_value::empty_conv (type.substr (2), rhs); - else - { - Cell& cell_ref = map.contents (key); + next_idx.erase (next_idx.begin ()); + next_idx.erase (next_idx.begin ()); - octave_value u1 = cell_ref.index (idx.front (), true); - u = numeric_conv (u1, type.substr (2)); + std::string next_type = type.substr (2); + + // cast map to const reference to avoid forced key insertion. + Cell tmpc = cmap.contents (key).index (idx.front (), true); + tmpc.make_unique (); - if (u.is_defined () && u.is_copy_of (u1)) + // FIXME: better code reuse? cf. octave_cell::subsasgn and the case below. + if (! error_state) + { + if (rhs.is_cs_list ()) { - // This is a bit of black magic. u is a shallow copy - // of an element inside this struct, and maybe more. To - // prevent make_unique from always forcing a copy, we - // temporarily delete the stored value. - u1 = octave_value (); - cell_ref.assign (idx.front (), Cell (octave_value ())); - u.make_unique (); - cell_ref.assign (idx.front (), Cell (u)); + octave_value_list rhsl = rhs.list_value (); + if (tmpc.numel () == rhsl.length ()) + { + for (octave_idx_type k = 0; k < tmpc.numel () && ! error_state; k++) + { + octave_value tmp = tmpc (k); + if (! tmp.is_defined () || tmp.is_zero_by_zero ()) + { + tmp = octave_value::empty_conv (next_type, rhs); + tmp.make_unique (); // probably a no-op. + } + else + // optimization: ignore the copy still stored inside our map and in tmpc. + tmp.make_unique (2); + + tmpc(k) = tmp.subsasgn (next_type, next_idx, rhsl(k)); + } + + t_rhs = octave_value (octave_value_list (tmpc), true); + } + else + error ("invalid cs-list length in assignment"); + } + else if (tmpc.numel () == 1) + { + octave_value tmp = tmpc(0); + + if (! tmp.is_defined () || tmp.is_zero_by_zero ()) + { + tmp = octave_value::empty_conv (type.substr (1), rhs); + tmp.make_unique (); // probably a no-op. + } + else + // optimization: ignore the copy still stored inside our map and in tmpc. + tmp.make_unique (2); + + if (! error_state) + t_rhs = tmp.subsasgn (next_type, next_idx, rhs); } else - // Safe is safe. - u.make_unique (); - - } - - if (! error_state) - { - std::list next_idx (idx); - - // We handled two index elements, so subsasgn to - // needs to skip both of them. - - next_idx.erase (next_idx.begin ()); - next_idx.erase (next_idx.begin ()); - - t_rhs = u.subsasgn (type.substr (2), next_idx, rhs); + error ("invalid assignment to cs-list outside multiple assignment."); } } else @@ -272,39 +291,64 @@ std::string key = key_idx(0).string_value (); - octave_value u; + std::list next_idx (idx); + + next_idx.erase (next_idx.begin ()); - if (! map.contains (key)) - u = octave_value::empty_conv (type.substr (1), rhs); - else - { - Cell& cell_ref = map.contents (key); + std::string next_type = type.substr (1); - u = numeric_conv (cell_ref, type.substr (2)); + Cell tmpc1 = octave_value (); + Cell& tmpc = (map.contains (key)) ? map.contents (key) : tmpc1; + + tmpc.make_unique (); - if (u.is_defined () && u.is_copy_of (cell_ref(0))) + // FIXME: better code reuse? + if (! error_state) + { + if (rhs.is_cs_list ()) { - // This is a bit of black magic. u is a shallow copy - // of an element inside this struct, and maybe more. To - // prevent make_unique from always forcing a copy, we - // temporarily delete the stored value. - cell_ref(0) = octave_value (); - u.make_unique (); - cell_ref(0) = u; + octave_value_list rhsl = rhs.list_value (); + if (tmpc.numel () == rhsl.length ()) + { + for (octave_idx_type k = 0; k < tmpc.numel () && ! error_state; k++) + { + octave_value tmp = tmpc (k); + if (! tmp.is_defined () || tmp.is_zero_by_zero ()) + { + tmp = octave_value::empty_conv (next_type, rhs); + tmp.make_unique (); // probably a no-op. + } + else + // optimization: ignore the copy still stored inside our map. + tmp.make_unique (1); + + tmpc(k) = tmp.subsasgn (next_type, next_idx, rhsl(k)); + } + + t_rhs = octave_value (octave_value_list (tmpc), true); + } + else + error ("invalid cs-list length in assignment"); + } + else if (tmpc.numel () == 1) + { + octave_value tmp = tmpc(0); + + if (! tmp.is_defined () || tmp.is_zero_by_zero ()) + { + tmp = octave_value::empty_conv (type.substr (1), rhs); + tmp.make_unique (); // probably a no-op. + } + else + // optimization: ignore the copy still stored inside our map. + tmp.make_unique (1); + + if (! error_state) + t_rhs = tmp.subsasgn (next_type, next_idx, rhs); } else - // Safe is safe. - u.make_unique (); - } - - if (! error_state) - { - std::list next_idx (idx); - - next_idx.erase (next_idx.begin ()); - - t_rhs = u.subsasgn (type.substr (1), next_idx, rhs); - } + error ("invalid assignment to cs-list outside multiple assignment."); + } } break; @@ -327,6 +371,7 @@ { std::list::const_iterator p = idx.begin (); octave_value_list key_idx = *++p; + octave_value_list idx_front = idx.front (); assert (key_idx.length () == 1); @@ -334,15 +379,36 @@ if (! error_state) { - map.assign (idx.front (), key, t_rhs); + if (t_rhs.is_cs_list ()) + { + map.assign (idx.front (), key, Cell (t_rhs.list_value ())); - if (! error_state) - { - count++; - retval = octave_value (this); - } - else - gripe_failed_assignment (); + if (! error_state) + { + count++; + retval = octave_value (this); + } + else + gripe_failed_assignment (); + } + else + { + // cast map to const reference to avoid forced key insertion. + if (idx_front.all_scalars () + || cmap.contents (key).index (idx_front, true).numel () == 1) + { + map.assign (idx_front, key, t_rhs.storable_value ()); + if (! error_state) + { + count++; + retval = octave_value (this); + } + else + gripe_failed_assignment (); + } + else if (! error_state) + error ("invalid assignment to cs-list outside multiple assignment."); + } } else gripe_failed_assignment (); diff --git a/src/ov-struct.h b/src/ov-struct.h --- a/src/ov-struct.h +++ b/src/ov-struct.h @@ -75,7 +75,7 @@ octave_value_list subsref (const std::string&, const std::list&, int); - static octave_value numeric_conv (const Cell& val, + static octave_value numeric_conv (const octave_value& val, const std::string& type); octave_value subsasgn (const std::string& type, diff --git a/src/ov.h b/src/ov.h --- a/src/ov.h +++ b/src/ov.h @@ -298,6 +298,19 @@ } } + // This uniquifies the value if it is referenced by more than a certain + // number of shallow copies. This is useful for optimizations where we + // know a certain copy, typically within a cell array, to be obsolete. + void make_unique (int obsolete_copies) + { + if (rep->count > obsolete_copies + 1) + { + --rep->count; + rep = rep->clone (); + rep->count = 1; + } + } + // Simple assignment. octave_value& operator = (const octave_value& a) diff --git a/src/pt-idx.cc b/src/pt-idx.cc --- a/src/pt-idx.cc +++ b/src/pt-idx.cc @@ -391,6 +391,12 @@ return retval; } +static void +gripe_invalid_inquiry_subscript (void) +{ + error ("invalid dimension inquiry of a non-existent value"); +} + octave_lvalue tree_index_expression::lvalue (void) { @@ -410,8 +416,6 @@ { bool have_new_struct_field = false; - octave_idx_type new_struct_field_nel = 0; - // I think it is OK to have a copy here. const octave_value *tro = retval.object (); @@ -441,10 +445,15 @@ // that argument list so we pass the appropriate // value to the built-in __end__ function. - octave_value_list tmp_list - = first_retval_object.subsref (type.substr (0, i), idx, 1); + if (first_retval_object.is_defined ()) + { + octave_value_list tmp_list + = first_retval_object.subsref (type.substr (0, i), idx, 1); - tmp = tmp_list(0); + tmp = tmp_list(0); + } + else + gripe_invalid_inquiry_subscript (); if (error_state) break; @@ -464,23 +473,27 @@ idx.push_back (tidx); - if (i == n-1) + if (! tidx.all_scalars () && retval.numel () == 1) { - // Last indexing element. Will this result in a - // comma-separated list? + // Possible cs-list. if (tidx.has_magic_colon ()) { - octave_value_list tmp_list - = first_retval_object.subsref (type, idx, 1); + if (first_retval_object.is_defined ()) + { + octave_value_list tmp_list + = first_retval_object.subsref (type, idx, 1); - if (! error_state) - { - octave_value val = tmp_list(0); + if (! error_state) + { + octave_value val = tmp_list(0); - if (val.is_cs_list ()) - retval.numel (val.numel ()); - } + if (val.is_cs_list ()) + retval.numel (val.numel ()); + } + } + else + gripe_invalid_inquiry_subscript (); } else { @@ -504,110 +517,109 @@ case '.': { octave_value tidx = get_struct_index (p_arg_nm, p_dyn_field); + if (error_state) + break; - if (! error_state) - { - if (i == n-1) - { - // Last indexing element. Will this result in a - // comma-separated list? + if (i > 0 && type [i-1] == '(' && retval.numel () == 1 + && ! idx.back ().all_scalars ()) + { + // Possible cs-list. + + std::string ttype = type.substr (0, i); - if (have_new_struct_field) - retval.numel (new_struct_field_nel); - else if (i > 0) - { - std::string ttype = type.substr (0, i); + octave_value_list xidx = idx.back (); - char c = ttype[ttype.length()-1]; - if (c == '(' || c == '{') - { - octave_idx_type nel = 1; + if (xidx.has_magic_colon ()) + { + if (first_retval_object.is_defined () && ! have_new_struct_field) + { + octave_value_list tmp_list + = first_retval_object.subsref (ttype, idx, 1); - octave_value_list xidx = idx.back (); + if (! error_state) + { + octave_value val = tmp_list(0); - octave_idx_type nidx = xidx.length (); + if (val.is_map ()) + retval.numel (val.numel ()); + } + } + else + gripe_invalid_inquiry_subscript (); + } + else + { + octave_idx_type nel = 1; - for (octave_idx_type j = 0; j < nidx; j++) - { - octave_value val = xidx(j); - - nel *= val.numel (); - } + octave_idx_type nidx = xidx.length (); - retval.numel (nel); - } - else if (first_retval_object.is_defined () - && ! (first_retval_object.is_real_matrix () - && first_retval_object.is_zero_by_zero ())) - { - octave_value_list tmp_list - = first_retval_object.subsref (ttype, idx, 1); + for (octave_idx_type j = 0; j < nidx; j++) + { + octave_value val = xidx(j); + + nel *= val.numel (); + } - if (! error_state) - { - octave_value val = tmp_list(0); + retval.numel (nel); + } + } + else if (retval.numel () == 1 && first_retval_object.is_defined ()) + { + octave_value tobj = first_retval_object; + + std::string ttype = type.substr (0, i); - retval.numel (val.numel ()); - } - } - else - retval.numel (1); - } - else - { - if (first_retval_object.is_defined () - && ! (first_retval_object.is_real_matrix () - && first_retval_object.is_zero_by_zero ())) - retval.numel (first_retval_object.numel ()); - else - retval.numel (1); - } - } - else - { - octave_value tobj = first_retval_object; + if (i > 0) + { + // Here we need to ensure that keys do exist. + + octave_value_list tmp_list + = first_retval_object.subsref (ttype, idx, 1); - if (! have_new_struct_field) - { - if (i > 0 && first_retval_object.is_defined () - && ! (first_retval_object.is_real_matrix () - && first_retval_object.is_zero_by_zero ())) - { - std::string ttype = type.substr (0, i); + if (tmp_list.length () > 0) tobj = tmp_list (0); + } - char c = ttype[ttype.length()-1]; + + std::string key = tidx.string_value (); - if (! (c == '(' || c == '{')) - { - octave_value_list tmp_list - = first_retval_object.subsref (ttype, idx, 1); - - if (! error_state) - tobj = tmp_list(0); - } - } - - if (! error_state && tobj.is_map ()) - { - if (tidx.is_string ()) - { - Octave_map m = tobj.map_value (); + if (! error_state) + { + if (tobj.is_map ()) + { + Octave_map map = tobj.map_value (); + if (map.contains (key)) + retval.numel (map.contents (key).numel ()); + else + { + map.contents (key) = octave_value (); + if (i > 0) + first_retval_object = + first_retval_object.subsasgn (ttype, idx, map); + else + first_retval_object = map; - std::string s = tidx.string_value (); - - if (! m.contains (s)) - { - have_new_struct_field = true; + have_new_struct_field = true; + } + } + else + { + Octave_map map (key, octave_value ()); + if (i > 0) + first_retval_object = + first_retval_object.subsasgn (ttype, idx, map); + else + first_retval_object = map; - new_struct_field_nel = m.numel (); - } - } - } - } - } + have_new_struct_field = true; + } + } + } - idx.push_back (octave_value (tidx)); - } + if (! error_state) + idx.push_back (tidx); + else + break; + } break; @@ -625,6 +637,7 @@ if (! error_state) retval.set_index (type, idx); + } return retval;