Re: Using C++ exceptions instead of error_state

[John W. Eaton]

On 12/22/2014 12:05 PM, Rik wrote:
> On 12/22/2014 03:03 AM, jwe wrote:

Thanks for the detailed comments.

> 1) Are there functions which set error_state, but that do not generate
> error messages and thus do not shortcircuit?
>
> I'm thinking of patterns in input validation of the form
>
> std::string arg = arg(0).string_value ();
> if (! error_state)
>   ...
> else
>    error ("ARG 0 must be a string");

I think code like this currently generates two error messages if 
conversion to a string is not possible.  One from the string_value 
function and one from the other call to error.  The only way error_state 
was supposed to be set was with a call to error, and I don't think there 
were many (any?) calls with empty message arguments in core Octave.

> This could be simplified to a single line if the string_value() function
> issues an error when the input cannot be coerced to a string.  However,
> it might make coding difficult if you don't know which functions will
> issue errors and shortcircuit and which will merely set the error_state
> variable.

With the changes I'm proposing, no function will set error_state.  All 
error handling will happen with exceptions.  Although not enforced, the 
only way that error_state was supposed to be set previously was by 
calling some form of error function.  Of course any function could call 
error, but if you didn't account for error_state, you could easily keep 
processing using invalid data.  Now you won't be able to do that unless 
you explicitly catch the exception.  However, I don't think we want to 
have a lot of try/catch blocks just for specific error messages.

> A secondary concern is that you lose the specificity of the error
> message.  Of course the code above could be re-written as
>
> if (! arg(0).is_string ())
>     error ("ARG 0 must be a string");
>
> std::string arg = arg(0).string_value ();
>
> which would preserve the specificity of the message.

Yes, there are likely some cases like this.  Should we consider a 
nargchk or inputParser functionality for the C++ code?  I don't know. 
Maybe for common cases like this we can consider some utility functions 
that will allow more specific error messages when they are needed.

> 2) Should we re-consider the base Octave C++ function style when making
> this change?

Yes, but I think we should do that as a separate change if we decide 
that it is worth doing.

> Example:
>
> if (nargin == 1)
>    {
>      if (arg(0).is_numeric ())
>        {
>          if (arg(0).ndims () == 2)
>            {
>               ... CODE ...
>               ... Possibly > 100 lines ...
>               ... CODE ...
>            }
>          else
>            error ("ARG must be a 2D matrix");
>        }
>      else
>        error ("ARG must be numeric")
>    }
> else
>    print_usage ();
>
> I find the C++ style difficult to follow because the error messages may
> be 100's of lines away from the tests.  Studies show that programmers
> have a hard time when a function can't fit on a single screen and the
> current style nearly guarantees that.

I understand that and am willing to change the style.  OTOH, in defense 
of the current style, I've preferred it because to me it is clearer 
about the conditions that are required for success.  I.e., "to compute 
this result, the following conditions must be met" instead of "this 
(possibly incomplete) set of conditions will generate errors".

Note that since there are no checks of error_state in your example 
above, no changes are actually needed for it to work with the changes 
I'm proposing.  I'd say we leave those alone for now to not risk 
breaking things that are known to work.  Later we can consider changing 
the style of these.

I'd also argue that our first round of changes should just remove the 
error_state checks and not do too much rearrangement of code.  To me, 
that seems like the safest way to make large scale changes like this. 
Then we can address style issues separately and systematically.

> I also find that by the time the code is ready to calculate, nearly 1/8
> to 1/4 of the screen has been used up with indentation which then
> requires lots of breaking of long lines at strategic points.

Yes, that is definitely a problem.

> 3) Unnecessary changes to m-files?
>
> The patch had changes to skewness.m, kurtosis.m, and test.m.  Was this
> intentional or the result of script search/replace?

Not intentional.  I was planning to check those changes in separately.

> In skewness.m, for example, the original code is
>
> ## Verify no "divide-by-zero" warnings
> %!test
> %! wstate = warning ("query", "Octave:divide-by-zero");
> %! warning ("on", "Octave:divide-by-zero");
> %! unwind_protect
> %!   lastwarn ("");  # clear last warning
> %!   skewness (1);
> %!   assert (lastwarn (), "");
> %! unwind_protect_cleanup
> %!   warning (wstate, "Octave:divide-by-zero");
> %! end_unwind_protect
>
> and the delta is
>
> --- a/scripts/statistics/base/skewness.m
> +++ b/scripts/statistics/base/skewness.m
> @@ -157,7 +157,7 @@
>   %!   skewness (1);
>   %!   assert (lastwarn (), "");
>   %! unwind_protect_cleanup
> -%!   warning (wstate, "Octave:divide-by-zero");
> +%!   warning (wstate);
>   %! end_unwind_protect
>
> But this doesn't seem right, because wstate will be "on" or "off", not a
> structure of all the preserved values.

Then there is a different bug because I'm currently seeing this:

octave:1> warning ("query", "Octave:str-to-num")
ans =

  scalar structure containing the fields:

    identifier = Octave:str-to-num
    state = off

If it is supposed to just return a string then I guess it has been 
broken for a while since I also see this behavior for 3.8.2.

jwe