Re: Archetype for C++ function

[John W. Eaton]

On 12/06/2015 11:07 PM, Rik wrote:
> 12/6/15
>
> jwe,
>
> The switch to exceptions in the core is making the code a lot clearer, in
> my mind anyways.  I want to propose a new archetype for C++ functions, and
> if you like it, we can possibly have it as one of the code sprint topics.
>
> The old archetype was
>
> [...]
>
> Now that error() short-circuits it makes sense to put the input validation
> first.  But the declaration "octave_value retval;" was also part of this
> structure because the original template would always reach the final
> "return retval;" statement.  Since that is no longer true, I think it makes
> sense to transition from the 'C' style of declaring all variables used
> anywhere in the function at the top, to the 'C++' style of declaring
> variables as they are needed in the narrowest possible local scope.

Yes, I agree.  The retval variable was really the thing given this 
treatment.  Other variables should have been declared as close as 
possible to first use.

> In this same vein, I wouldn't declare variables unnecessarily and often
> 'nargin' is used just once in the input validation.

Yes.  I started to do some of that when I was working on the print_usage 
changes, but decided to mostly focus on print_usage instead of things 
like nargin being used only once.  Though I did fix that in some cases.

> For these cases I
> would just code "args.length ()" directly.  Here is the proposed new
> archetype for the same code sample as above.
>
> --- Archetype 2 ---
> if (args.length () != 2)
>    print_usage ();
>
> NDArray x = args(0).xarray_value ("first argument must be a numeric array");
> std::string opt = args(1).xstring_value ("second argument must be a string");
>
> octave_value retval;
>
> // Do algorithm
> ...
> ...
> ...
> ...
>
> return retval;
> --- End Code ---
>
> This leads to some very tight pieces of code.

Yes, and I would also eliminate retval if you end up with something like

  octave_value retval;

  ...

  retval = some_thing ();

  return retval;

> For example, the rows
> function in data.cc is coded today as
>
> --- rows DEFUN ---
> if (args.length () != 1)
>    print_usage ();
>
> return octave_value (args(0).rows ());
> --- End Code ---
>
> Where args.length() is used multiple times I would define the variable
> nargin since it makes the code more readable.  For example, the DEFUN for
> complex() in data.cc is currently written as
>
> --- complex DEFUN ---
> int nargin = args.length ();
>
> if (nargin < 1 || nargin > 2)
>    print_usage ();
>
> if (nargin == 1)
>    {
>      ...
>      ...
>    }
> else
>    {
>      ...
>      ...
>    }
> --- End Code ---

I agree, we should not repeat the call to args.length () unnecessarily. 
 Though it doesn't really matter as it is declared const and I think 
the compiler is free to call it just once anyway.

> In browsing through the code I see there has been some confusion as to what
> data type to use for nargin.  The two most common templates are
>
> --- nargin templates ---
> int nargin = args.length ();
> octave_idx_type nargin = args.length ();
> --- End Code ---
>
> I propose standardizing on the first since it is shorter.

Using int should be OK as I don't think it is reasonable for it to ever 
be larger than 2^31.  At least I hope no one ever needs that.  Can you 
imagine a case where that could be true?

> Finally, I think you had a preference in the m-files not to use length()
> when what was actually desired was the number of elements.

I did, but that's because of the funny definition of length in Matlab.
> Currently the
> octave_value_list() class does not have a numel() function.  This is ironic
> since the length() member function actually calls numel() internally.
> Would it make sense to add a numel member function so that one could write
>
> int nargin = args.numel ();

The octave_value_list type is always a one-dimensional object, so I 
think using length is OK, similar to using length for std::string.  So 
in this case I think it might be best to leave it as is instead of 
adding another name for the same thing.

jwe