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

[David Spies]

On Fri, Aug 8, 2014 at 12:39 PM, John W. Eaton <address@hidden> wrote:

On 08/08/2014 12:21 PM, David Spies wrote:

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

<mailto: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
    <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.

That ID was what hg id told me.  I wasn't looking at the diff, but that was the revision I was standing on when I was reading the code.  I thought it would make the most sense to comment on the current state of the code, rather than trying to comment on a particular diff.

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.

Then I guess I don't really see the point of the namespace.  You might as well just prefix things with ffind_ and find_.

To help clarify what's private.  (C++ doesn't have any way to do this outside of the OO paradigm)

         // 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.

How is the type of resvec unknown?  It appears to me that the type of resvec is R.

R is a template parameter.  That's what I meant by unknown

    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.

But putting stuff in a namespace doesn't actually make it private, so if you really want to have all these internal/private functions, maybe some other mechanism is appropriate?

Not in C++.  We already had this whole discussion and I thought the final decision was that the best approach would be to use a namespace to conceal the "private" functions.

It's named find_to_R because it
does the find logic and stores the result in something of type R.

There has to be a better name than "find_to_R".  Just "find" would be better.  Most functions do something and store the result in some object, but they are not all named VERB_to_OBJECT and naming something that way does not help with understanding.

There already is a "find".

    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)

As it is, your two switch cases are nearly identical copy-pasted code. Perhaps I'm wrong, but I think there has to be a better way of writing this.

Three lines of copy-pasted boilerplate right next to each other is a very different thing from copy-pasting the entire find function.

    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").

The "F" prefix on the defun functions isn't really something that I would like to expose or base other function or object names on.

How should I distinguish between them then?

    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.

Then let's try to find better names that don't use "to" this way.

       {
         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.

The attached patch seems to work for me, so it appears to that returning resvec is possible.

Oh yeah. I was originally using the different constructor parameters for find_result and ffind_result.  I forgot I had changed it so that they both take a dim_vector.

       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.

I don't really follow this, because couldn't you just create an iterator object and pass that to the function rather than have it as a template parameter too?  Then your function that uses the iterator object would just ask the iterator for the position, and to move to the next element.  Why does it need to know the direction of iteration? 

Because that way the iterator can be constructed within the function itself.  It's one less line of code to be duplicated on the outside.