Re: overloaded functions vs. internals

[Jaroslav Hajek]

On Wed, Oct 7, 2009 at 10:09 PM, Jaroslav Hajek <address@hidden> wrote:
> On Wed, Oct 7, 2009 at 8:39 PM, John W. Eaton <address@hidden> wrote:
>> Given the following class constructor and method:
>>
>> address@hidden/foo.m:
>>  function x = foo ()
>>    x = class (struct (), 'foo');
>>  end
>>
>> address@hidden/size.m:
>>  function s = size (x)
>>    s = [13, 42];
>>  end
>>
>> Octave will do the following:
>>
>>  octave> x = foo ();
>>  octave> size (x)
>>  ans =
>>
>>     13   42
>>
>>  octave> whos x
>>  Variables in the current scope:
>>
>>    Attr Name        Size                     Bytes  Class
>>    ==== ====        ====                     =====  =====
>>         x           1x1                          0  foo
>>
>> The problem is that internally, whos is computing size using the
>> octave_value::dims function, and that does not call size, even though
>> the octave_value::size function has been overloaded to call a
>> user-defined size method if it is available.
>>
>> Fixing whos to call size instead of using dims, and then construct the
>> dimension string from that fixes the problem for whos.  As a quick fix
>> for the person who reported this problem to me, I checked in the
>> following change:
>>
>>  http://hg.savannah.gnu.org/hgweb/octave/rev/bb413c0d0d6d
>>
>> However, the problem remains for any other internal function that
>> calls dims on a class object that has an overloaded size function.
>>
>> I started working on a better fix, but ran into problems with const.
>> For example, I would like to replace the current octave_class::dims
>> function with
>>
>>  dim_vector
>>  octave_class::dims (void) const
>>  {
>>    // Be consistent with size if it is overloaded with a user-defined
>>    // method.
>>
>>    Matrix sz = size ();
>>
>>    dim_vector dv (sz.numel ());
>>
>>    for (int i = 0; i < dv.length (); i++)
>>      dv(i) = sz(i);
>>
>>    return dv;
>>  }
>>
>> but this won't work because size is not const (and can't be; see
>> below).  Dropping the const qualifier from the dims function is not
>> desirable, because dims is used in many other const functions, so it
>> would cause a cascade of const removal.  I don't think we want that.
>>
>> So why isn't the octave_value::size method constant?  Because the
>> octave_class definition of size is
>>
>>  Matrix
>>  octave_class::size (void)
>>  {
>>    Matrix retval (1, 2, 1.0);
>>    octave_value meth = symbol_table::find_method ("size", class_name ());
>>
>>    if (meth.is_defined ())
>>      {
>>        count++;
>>        octave_value_list args (1, octave_value (this));
>>
>>        octave_value_list lv = feval (meth.function_value (), args, 1);
>>        if (lv.length () == 1 && lv(0).is_matrix_type () && lv(0).dims 
>> ().is_vector ())
>>          retval = lv(0).matrix_value ();
>>        else
>>          error ("@%s/size: invalid return value");
>>      }
>>
>>    return retval;
>>  }
>>
>> Note the lines
>>
>>        count++;
>>        octave_value_list args (1, octave_value (this));
>>
>> This sets up the argument list for the call to the user-defined
>> method.  We have to increment the reference count of the octave_class
>> object when we put it inside a new octave_value object so that it can
>> go in the argument list.  Since count is a member of
>> octave_base_value, and octave_class is derived from that,
>> octave_class::size can't be const.
>>
>> My next thought was to make count mutable, but that is not sufficient
>> because if octave_value::size is const, then "this" is const inside
>> octave_value::size, and although we have an
>>
>>  octave_value::octave_value (octave_base_value *)
>>
>> constructor, there is no
>>
>>  octave_value::octave_value (const octave_base_value *)
>>
>> constructor because octave_value::rep is not const and copying the
>> const argument to the non-const rep won't work.  And anyway, rep can't
>> be const if we want to call any non-const member functions through the
>> rep pointer.
>>
>> So, I'm a bit stuck here.  Any thoughts about how to get out of this
>> mess?
>>
>> Ideally, I'd like to find a solution for the const problem so that we
>> can call overloaded methods without having to give up const.  Perhaps
>> there is something simple that I'm missing, but I just don't see a
>> solution at the moment.
>>
>
> I mostly thought along the same lines when I was improving
> octave_class - that's why I decided to create a separate non-const
> size() method (previously, octave_value::size existed but only called
> dims() and thus wasn't much useful), that will behave correctly even
> with classes. It returns a Matrix because the original
> octave_value::size did so and also because a user-defined size()
> method may actually return something more complicated than a canonical
> dim_vector.
>
> Btw., for the same reason there is numel (void) const and numel (const
> octave_value_list&), the latter calling overloaded numel() for classes
> but the former not.
>
> This seemed to me as the least painful approach, because I found out
> the same difficulties as you did.
> Currently, IMHO the only const-clean approach to do what you would
> like to is to construct
> octave_value (this->clone ()) and pass that to the method. That
> involves some overhead of duplicating the object's internals - map and
> parent_list. map is reference counted, so copying it is actually quite
> cheap, parent_list is worse, but probably easily acceptable.
> A dirty alternative, of course, is going via const_cast.
>
> I didn't do it myself when thinking of it because I wasn't so sure it
> is a good idea to let dims() call overloaded size() method. I think an
> infinite recursion might easily occur, given how often dims() is
> called. There should be some mechanism to get the internal dims, and
> to avoid the recursion. map_value ().dims () will work, but is not
> very nice.
>
> Generally, I think one usually wants to handle classes as a separate
> case anyway, so it doesn't hurt much to have separate methods. For
> instance, currently dims() never generates an error; that would also
> change. But I don't really object to such changes, just pointing out
> possible problems.
> The same solution and similar arguments apply to the other const
> methods you refer to below.
>
> regards
>

In fact I realized that I myself made exactly the mistake I was just
warning against:
http://hg.savannah.gnu.org/hgweb/octave/rev/5acd99c3e794

the problem is that octave_value::numel (const octave_value_list&)
can, unlike other methods, call a user-defined class method, or even
user-defined size() method. But you don't want to call it from within
the builtin numel, because then if you called
builtin("numel",obj) from within the overloaded numel for obj, it
would end up in an infinite recursion.
Another possibility would be to employ a similar trick like the one
within octave_class::subsref and octave_class::subsasgn and check for
called_from_builtin inside octave_class::numel (const
octave_value_list&).
But I don't like that approach very much - it can be fooled around,
though with some difficulty.

In short, when calling an octave_value::some_method from C++, there is
no magic to tell whether you mean some_method(obj,...) or
builtin("some_method", obj). Currently, most of the octave_value
methods correspond to the second form (i.e. builtin), except a small
set of methods intimately connected to indexing: size(), numel (const
octave_value_list&), subsref, subsasgn.
(Note that all of them are non-const).
While the former two have their built-in-only counterpart (basically,
dims()), the latter two check the call stack to determine which form
is meant. That is convenient and will sometimes do just the right
thing, but in other cases it can fail. Even now it's not bulletproof:
for instance, using eval("object.field") inside a class subsref method
while you're handling object.field triggers an infinite recursion, but
maybe we can improve it.
It would be even better if we had subsref/subsref_with_overload
subsasgn/subsasgn_with_overload or something like that, but that would
require massive changes now.

regards

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