Re: Fwd: Considering adding a "dispatch" function for compile-time polymorphism

[David Spies]

On Thu, Aug 7, 2014 at 1:25 PM, John W. Eaton <address@hidden> wrote:

I can see the value of having a unified approach to dispatching on
type, but I'm a bit reluctant to accept your current solution because
it seems to add considerable complexity that I find somewhat hard to
follow.  For example, I find a number of the function and object names
confusing, so maybe improving those would make the code easier for me
to follow.

Here are some specific comments.  I'm looking at revision c689ac34114d
in your archive at http://hg.octave.org/octave-dspies.

I think you're looking at a different revision.  You seem to be addressing the revision that makes changes to find, not the one that adds the dispatch function.  Perhaps we should be discussing it in a separate email thread.

Also, it's probably best to identify it by comment rather than by revision hash since that changes with every rebase.

In find.h:

  namespace find
  {
    [...]

    struct find_result

Since this object is inside the namespace find, is it necessary for it
to be called "find_result"?  Maybe just "result"?

find_result is used to store the result type of the "find" function.  Later ffind_result is used to store the result type of the Ffind function.  The names distinguish between the two.
 

    // This is the generic "find" method for all matrix types.  All calls to
    // "find" get routed here one way or another as this method contains logic
    // for determining the proper output dimensions and handling forward/reversed,
    // n_to_find etc.
    // It would be unwise to try and duplicate this logic elsewhere as it's fairly
    // nuanced and unintuitive (but Matlab compatible).

    template<direction dir, typename MT, typename R>
    void
    find_to_R (dir_handler<dir> dirc, const MT& v, octave_idx_type n_to_find,
               R& resvec)

If this function creates/fills resvec, why not just return resvec
instead of passing it by reference?

Because the type of resvec is unknown.  The way in which resvec is created depends on the caller.

What does "find_to_R" mean?  This name is confusing to me.  If this is
the 'generic "find" method', then why not just call it "find"?  Is
this function supposed to be called by users, or is it really an
internal/private function?

Yes, this is an internal/private function as is everything in the find namespace except for "find" itself.  It's named find_to_R because it does the find logic and stores the result in something of type R.

    // The method call used for internal liboctave calls to find.  It constructs
    // an find_result and fills that rather than a res_type_of (see "find.cc").
    // Note that this method only supports full arrays because octave_idx_type
    // will generally overflow if used to represent the nonzero indices of sparse
    // arrays. Instead consider using the sparse-iterator class from
    // nz-iterators.h to avoid this problem

    template<typename ELT_T>
    Array<octave_idx_type>
    find (const Array<ELT_T>& v, octave_idx_type n_to_find = -1, direction dir =
              FORWARD)
    {
      find_result resvec;
      switch (dir)
        {
        case FORWARD:
          dir_handler<FORWARD> dirc_forward;
          find_to_R (dirc_forward, v, n_to_find, resvec);
          break;
        case BACKWARD:
          dir_handler<BACKWARD> dirc_back;
          find_to_R (dirc_back, v, n_to_find, resvec);
          break;
        default:
          liboctave_fatal ("find: unknown direction: %d; must be 1 or -1", dir);
        }

      return resvec.res;
    }
  }

Would it be better to have find and rfind functions to handle the
forward and backward find operations?

No, because those would be two functions with nearly identical logic (ie copy-pasted).  Here instead there's only one function (which avoids copy-pasted code)
 

If not, and the direction parameter is needed, then is it necessary to
bother with the default case here?  Since direction is an enum, can it
ever have values other than forward or backward?

I think the default case deals with compiler complaints.  It could theoretically occur if someone did an unchecked cast from int to dir.
 

It might also be better to report forward and backward instead of 1

and -1 in the error message.

OK

In find.cc:

namespace find
{
  [...]

  template<int nargout, typename T>
  struct ffind_result;

Is this called ffind because DEFUN generates a function called Ffind?

Yes

I guess I would just call it "find_result", though again, since this
is inside the "find" namespace, maybe it should just be called "result".

The naming convention is meant to distinguish between the result of the interpreter function "Ffind" and the C++ function "find" (in "find.h").

  // Calls the to_R template method in "find.h" which
  // in turn will fill in resvec of type ffind_result (see above).
  // This is generic enough to work for any matrix type M
  template<direction dir, int nargout, typename M>
  octave_value_list
  call (const M& v, octave_idx_type n_to_find)

I guess n_to_find is the number of elements to find?  That's somewhat
confusing given the other "find_to_R" function name.

Unfortunately the word "to" in English is heavily overloaded.

  {
    ffind_result<nargout, typename M::element_type> resvec;
    dir_handler<dir> dirc;
    find_to_R (dirc, v, n_to_find, resvec);

    return resvec.get_list ();
  }

It seems to me this could just be:

  result<nargout, typename M::element_type> resvec
    = find_to_R (dir_handler<dir>, v, num_elts_to_find);

  return resvec.get_list ();

Or maybe just

  find_to_R (dir_handler<dir>, v, num_elts_to_find) . get_list ();

find_to_R can't construct resvec because it doesn't know how.  That's why it's passed by reference rather than returned.
 

but then I'm left wondering why the "call" template function is needed.

  // A large annoying plethora of switch statements necessary to

I find comments like this to be annoying.  :-/

Ok

  template<int nargout, typename M>
  octave_value_list
  dir_to_template (const M& v, octave_idx_type n_to_find, direction dir)
  {
    switch (dir)
      {
      case BACKWARD:
        return call<BACKWARD, nargout> (v, n_to_find);
      case FORWARD:
        return call<FORWARD, nargout> (v, n_to_find);
      default:
        panic_impossible ();
      }
    return octave_value_list ();
  }

Names like dir_to_template don't tell me much.  The purpose of this
function is what?  To handle the fact that DIR can't be a template
parameter, so you switch over the possible values of DIR?

Exactly

Maybe we
should just have find and rfind functions instead?  Or simply pass the
direction of the find operation as an argument instead of generating
separate templates based on direction?  Why is it necessary to have
the direction be a template parameter?

Because nz_iterator requires a DIR template parameter to know how to step.

  template<typename M>
  octave_value_list
  nargout_to_template (const M& v, int nargout, octave_idx_type n_to_find,
                       direction dir)
  {
    switch (nargout)
      {
      case 1:
        return dir_to_template<1> (v, n_to_find, dir);
      case 2:
        return dir_to_template<2> (v, n_to_find, dir);
      case 3:
        return dir_to_template<3> (v, n_to_find, dir);
      default:
        panic_impossible (); // Checked by *** in Ffind
      }
    return octave_value_list ();
  }

This function seems to be used in only one place.  Is it necesary to
have it as a separate function?  Why not just put the switch inside
your "find_templated" function? 

Because that would destroy the naming conventions I've tried to set up and it would mix two different things into the same function.  find_templated is the function called once the argument's template type has been established.  nargout_to_template establishes the template argument for nargout, and dir_to_template establishes the template for dir.

  struct find_info
  {
    octave_idx_type n_to_find;
    direction dir;
    int nargout;
  };

  // This functor will be called by dispatch.h with the proper type M.
  // This avoids having to explicitly list the different types find can
  // handle and instead delegates that duty to the generic "dispatch"
  // function.
  template<typename M>
  struct find_templated
  {
    octave_value_list
    operator() (const M& v, const find_info& inf)
    {
      return nargout_to_template (v, inf.nargout, inf.n_to_find, inf.dir);
    }
  };

Why call this "find_templated"?  I don't think that naming functions
"templated" helps much with understanding what the functions are
doing.

 It's the find function where the parameter is passed as a template-type rather than as an octave_value

If you've packaged the find_info in a struct, why split it out here?
Why not continue to pass it down to other functions as a struct until
the individual elements are actually needed?

The struct is just a wrapper for the other arguments so that they can be passed to dispatch.  The arguments don't logically belong together as being anything besides "auxiliary arguments to find".