[major] struct array indexing in tip

[Jaroslav Hajek]

On Tue, Jan 20, 2009 at 11:06 PM, John W. Eaton <jwe at octave.org> wrote:
> On 20-Jan-2009, Jaroslav Hajek wrote:
>
> | On Mon, Jan 19, 2009 at 3:45 AM, Nicholas Tung <ntung at ntung.com> wrote:
> | > in revision
> | > hg tip
> | > changeset:   8534:0eb83938c8bc
> | >
> | > struct indexing is messed up. The following code should simply add to the
> | > value of c at 2, but creates some weird cell array...
> | > octave:1> a.c = [1, 2, 3]
> | > a =
> | > {
> | >   c =
> | >      1   2   3
> | > }
> | > octave:2> e = [a, a]
> | > e =
> | > {
> | >   1x2 struct array containing the fields:
> | >     c
> | > }
> | > octave:3> e(1).c(2)
> | > ans =  2
> | > octave:4> e(1).c(2) += 1
> | > e =
> | > {
> | >   1x2 struct array containing the fields:
> | >     c
> | > }
> | > octave:5> e.c
> | > ans =
> | > {
> | >   [1,1] =
> | >      1   2   3
> | >   [1,2] =  3
> | > }
> | > ans =
> | >    1   2   3
> | >
> | > Thanks in advance; please tell me when it's fixed.
> | >
> | > cheers,
> | > Nicholas
> |
> | Gosh, it took me about a day and half, and here goes:
> | http://hg.savannah.gnu.org/hgweb/octave/rev/3d8a914c580e
> |
> | Your bug seems fixed. Moreover, for instance, the following will now work:
> |
> | [a(1:2).x] = deal (1, 3)
> | [a(:).x(2)] = deal (5, 6)
> |
> | I'm still not convinced all cases are caught, so I encourage everyone
> | to experiment.
> | In fact, I'd be surprised if it were really bug-free, but make check
> | didn't catch anything.
>
> Thanks for looking at this problem.
>

Okay, I've seen a couple of bugs myself, so here's another shot:
http://hg.savannah.gnu.org/hgweb/octave/rev/906f976d35a8

This time, I didn't try to allow further indexing on cs-lists, like
a(1:2).x(1), because Matlab doesn't allow it either, and it
complicates things a little.

octave:1> [a(1:2).x(2)] = deal(3,4)
error: a cs-list cannot be further indexed
error: evaluating argument list element number 1

I've revamped the tree_index_expression::lvalue code to avoid useless
evaluation of the trailing indexing expr just for getting dimensions,
so things should be somewhat faster now. Also, re-evaluation of an
already evaluated portion of the index chain is avoided in both lvalue
and rvalue, so that the following code calls testfun only once:

function y = testfun (x)
  disp ('called');
  for i = 1:5
    y{i} = (1:i) * x;
  endfor
endfunction

testfun (5) {end} (end)

instead of 3 times (prior to this patch).

Also, I've disallowed automatic resizing in the normal subsref
function, so that the following code will now produce a proper error:

a(1).x.x = 1
a(2).x

>
> Maybe someone (other than Jaroslav) would like to generate some tests
> so these problems don't resurface later?
>

That would be great. I *may* do it eventually myself, but no promise :)

> jwe
>

I'd still like to make more changes to the indexing code:

First, an indexed assignment to an out-of-bounds element that does not
occur last in the indexing chain
does unnecessarily resize the array three times via the indexing with
resize_ok. I don't think that Array<T>::index (..., resize_ok) is
actually used by any instance than Cell (i.e. Array<octave_value>),
and probably the only correct usage is with scalar-resulting indices,
so I'd like to move the methods away from Array<T> to Cell and
optimize the all-scalars case to avoid the unnecessary resizes.

Second, given that octave_value_lists are often converted to Cells and
vice versa, I'd like to consider rebasing octave_value_list as a Cell
subclass. That would allow virtually all argument-passing code benefit
from the shallow copy mechanism of Array<T> (now even for contiguous
subranges), and thus reduce the amount of copying octave_values
around. That would allow things like func (args{:}) to work without
copying.

opinions?

-- 
RNDr. Jaroslav Hajek
computing expert
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz