Re: fix null assignment

[John W. Eaton]

On 19-Sep-2008, Jaroslav Hajek wrote:

| On Fri, Sep 19, 2008 at 6:50 PM, Jaroslav Hajek <address@hidden> wrote:
| > On Fri, Sep 19, 2008 at 5:50 PM, John W. Eaton <address@hidden> wrote:
| >> On 19-Sep-2008, Jaroslav Hajek wrote:
| >>
| >> | It seems that there is a need to propagate the information (whether
| >> | this is a deletion statement) fairly deeply - thinking of things like
| >> | a(3).x.y(1:2,:) = []
| >> | that should work... this stuff is resolved by subsasgn, and creating
| >> | methods like subsnulasgn would probably mean a lot of duplication. It
| >> | seems better to make a special kind of object for the "genuine" empty
| >> | matrix that was not touched by expressions. I'm just not sure where to
| >> | put it.
| >>
| >> I think there are a few things to consider.  First, for an expression
| >> like "X(I) = []", if X is a class object, the subsasgn method for the
| >> class is still called.  So in that case, if we have the parser
| >> recognize "= []" as a special case and convert it to a call to a
| >> function that deletes elements, then that function will need to
| >> dispatch to the subsasgn method for the given class.
| >>
| >> Another possibility is to tag matrices that are created with [], then
| >> use that information in the assign2 and assignN functions to decide
| >> when to call maybe_delete_elements.  So instead of
| >>
| >>  if (rhs_nr == 0 && rhs_nc == 0)
| >>    {
| >>      lhs.maybe_delete_elements (idx_i, idx_j);
| >>    }
| >>
| >> in the assign2 function in Array.cc, we would have something like
| >>
| >>  if (rhs.is_null_array ())
| >>    {
| >>      lhs.maybe_delete_elements (idx_i, idx_j);
| >>    }
| >>
| >> But is this the right place for the check?  Maybe not, since it only
| >> happens when the RHS is [] (i.e., double) and the LHS can be anything
| >> (even a struct array).  So it should probably be handled a little
| >> higher up, and the detection and call to maybe_delete_elements should be
| >> removed from the assign2 and assignN functions.  In that case, I think
| >> the proper places for this are the various octave_value subsasgn
| >> methods.  Then the special tag for the [] value could go in the
| >> octave_value object itself.  Perhaps there could even be a special
| >> octave_value type for this object, similar to the octave_magic_colon
| >> type.  In any case, the special nature of this value/type should not
| >> be propagated by assignment or copying since
| >>
| >>  x(idx) = []
| >>
| >> deletes, but
| >>
| >>  y = []; x(idx) = y
| >>
| >> does not.
| >>
| >> Unless someone else is interested, I'll take a look at making these
| >> changes.
| >>
| >
| > I was about to look at this; however, if you have a better idea, I'd
| > say you go ahead.
| >
| > My aim was to avoid checking for some special value in the
| > octave_value assignment operator & copy constructor (as you have said,
| > it should not propagate through by copying). octave_values are copied
| > around a lot and checking stuff there just doesn't seem nice to me.
| > My idea was different:
| > Add a new octave_value::assign_op (say, octave_value::op_nul_asn_eq)
| > and simply register proper handlers for it. Then, subclass
| > tree_null_matrix from tree_constant, and have the
| > tree_simple_assignment with etype = op_asn_eq mutate into
| > op_nul_asn_eq when it detects that the rhs is a tree_null_matrix
| > instance. The calls to maybe_delete_elements will be removed from
| > assignX and called directly.
| >
| > What do you think about this?
| 
| OK, I see this is not that easy. Currently, the various operators in
| octave_value::assign_op have no handlers actually registered; instead,
| they are always decomposed in octave_value::assign and they cannot be
| propagated down through subsagn & numeric_assign. This is even marked
| as a FIXME in ov.cc; perhaps it is time to do it now?
| 
| I'd like to know your opinion on this before I start to do anything
| with this matter.

I'd prefer to avoid adding a new assign_op.  I think we would have to
be careful to also check for op_nul_asn_eq in each place where
op_asn_eq is used and that is likely to cause confusion.

Please take a look at the two sets of diffs below.

The first set of changes seem to have the desired effect, but I'm not
entirely happy with them.

The second set was my attempt to move the nul_matrix flag up to just
the NDArray and charMatrix classes (that part is not complete) by
modifying the assignment operations so that [] (or "") on the RHS of
an assignment is trapped in the assignment operator itself, and then
calling maybe_delete_elements directly from there rather than from the
assignX functions.  This set of changes does not work properly.  For
example, given x = {1, 2, 3}, x(1) = []  produces {[], 2, 3} instead
of {2, 3}.

I'm tired of looking at this problem now.  I'd probably go for some
variation on the first change unless you have a better idea.

jwe

empty-assign-diffs-1.gz
Description: Binary data

empty-assign-diffs-2.gz
Description: Binary data