[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Archetype for C++ function
From: |
John W. Eaton |
Subject: |
Re: Archetype for C++ function |
Date: |
Mon, 07 Dec 2015 08:55:12 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.7.0 |
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