Re: Branching on builtin_type ()

[Jordi Gutiérrez Hermoso]

2012/3/5 John W. Eaton <address@hidden>:
> On  5-Mar-2012, Jordi Gutiérrez Hermoso wrote:
>
> | While reading Jaroslav's code, I frequently come across constructs
> | like this one:
> |
> |           switch (argx.builtin_type ())
> |             {
> |             case btyp_double:
> |               retval = argx.array_value ().nth_element (n, dim);
> |               break;
> |             case btyp_float:
> |               retval = argx.float_array_value ().nth_element (n, dim);
> |               break;
> |             case btyp_complex:
> |               retval = argx.complex_array_value ().nth_element (n, dim);
> |               break;
> |             case btyp_float_complex:
> |               retval = argx.float_complex_array_value ().nth_element (n, 
> dim);
> |               break;
> |     #define MAKE_INT_BRANCH(X) \
> |             case btyp_ ## X: \
> |               retval = argx.X ## _array_value ().nth_element (n, dim); \
> |               break
> |
> |             MAKE_INT_BRANCH (int8);
> |             MAKE_INT_BRANCH (int16);
> |             MAKE_INT_BRANCH (int32);
> |             MAKE_INT_BRANCH (int64);
> |             MAKE_INT_BRANCH (uint8);
> |             MAKE_INT_BRANCH (uint16);
> |             MAKE_INT_BRANCH (uint32);
> |             MAKE_INT_BRANCH (uint64);
> |     #undef MAKE_INT_BRANCH
> |             default:
> |               if (argx.is_cellstr ())
> |                 retval = argx.cellstr_value ().nth_element (n, dim);
> |               else
> |                 gripe_wrong_type_arg ("nth_element", argx);
> |             }
> |
> | This is from data.cc:6415 @ rev ec660526ae50.
> |
> | Can't this be improved? This manual polymorphism is error-prone
> | and ugly to read. Isn't there an abstract array type that one can
> | convert an octave_value into and then call virtual functions on
> | this type?
>
> I think the right thing to do is to change the above switch statement to
>
>  retval = argx.nth_element (n, dim);
>
> and then define
>
>  octave_value
>  octave_value::nth_element (const idx_vector& n, int dim) const
>  {
>    return rep->nth_element (n, dim);
>  }
[snip]
> I don't know of a simpler way to do this.

That's one thing I don't understand about octave_value. Doesn't this
seem like the wrong level of generality? An octave_value can also be a
function handle or a cell string. It seems wrong for those
octave_values to have an interface to all of these functions that
should only apply to numerical arrays. I wish there was an
intermediate type between octave_value and the numeric arrays that
Jaroslav is branching on... but perhaps that is too much work for no
real good reason.

> Mapper functions are similar, but reduce to repeated application of
> a simple non-member function. For those, we thought about doing the
> same as I outline above but decided to use a generic map function in
> the octave_value hierarchy and to switch on the type of map inside
> the map function for each specific value type (for example, see
> octave_matrix::map in ov-re-mat.cc).
>
> Maybe a similar strategy could be used for sum, cumsum, prod,
> cumprod, nth_element, etc. to abstract them all into one "reduction"
> function, but these functions ultimately call member functions, so
> I'm not sure how to handle that in a clean way with function
> pointers. Maybe there is a template trick I'm not seeing at the
> moment.

Isn't it just a matter of creating a friend reduction function, and
have that friend make the polymorphic calls through the rep pointer?

> How many of these functions are there that have this kind of switch
> statement?

It's probably not as many as I feared:

    address@hidden:~/coding/vcs/octave-devel$ grep -R 'define
MAKE_INT_BRANCH'  src/ liboctave/ | wc -l
    12

- Jordi G. H.