Re: Using C++ exceptions instead of error_state

[Rik]

On 12/22/2014 03:03 AM, address@hidden wrote:
> Subject:
> Using C++ exceptions instead of error_state
> From:
> "John W. Eaton" <address@hidden>
> Date:
> 12/22/2014 03:03 AM
> To:
> address@hidden
> List-Post:
> <mailto:address@hidden>
> Precedence:
> list
> MIME-Version:
> 1.0
> Message-ID:
> <address@hidden>
> Content-Type:
> multipart/mixed; boundary="------------070600000208080800020604"
> Message:
> 2
>
> I've been experimenting with having the error function throw a C++ exception instead of setting error_state.  The current draft of my patch is attached.  All the tests that were passing before the change appear to still be passing, so I think it is mostly functional.
>
> With these changes, you can simply write
>
>   if (condition)
>     error ("lookout!");
>
>   ... more code that should only execute if there is no error ...
>
> and the error message and stack trace will be generated and execution will jump to the top level magically.  There's no longer any need to check the value of error_state after operations that might generate error messages.  This change will make error handling in Octave's C++ code much more like what is done in .m files.

I like the consistency between m-file and C++ programming.  I also like the short-circuiting nature of the error statement.  There is lots of C++ code where one has to check carefully in the middle of a nested "if tree" whether control will be returned, without executing any further statements, to the end of the function where 'return retval;' is located.  Sometimes it isn't the case and you must add a 'return retval;' directly at the point where the error was generated.  Either way, it requires a lot of analysis to be able to modify the code which would be simplified if error() simply short-circuited.

Questions:

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

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.

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.

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

Correcting all 1600+ instances is going to mean opening up most of the C++ code.  As long as we are doing such a large change, it seems worthwhile to re-consider the basic style template for Octave functions.

Currently, the m-file syntax is
  1) Input validation w/error messages
  2) Calculation
  3) Output validation/conversion to desired output format or number of outputs

The C++ convention is
  1) Input checks
  2) Calculation
  3) Error messages
  4) Output validation/conversion to desired output format or number of outputs

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 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.  This would also be ameliorated by changing to the m-file style.  Re-writing the above example gets rid of 14 characters of indentation.

if (nargin != 1)
  print_usage ();

if (! arg(0).is_numeric ())
  error ("ARG must be numeric")
else if (arg(0).ndims () != 2)
  error ("ARG must be a 2D matrix");
 
CALCULATION CODE

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?

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.


--Rik

>
> The error_state variable remains because a lot of code still uses it. There are more than 1600 locations where it is used in core Octave that I have not yet fixed.
>
> Even though it will require quite a bit more work to finish this change (those 1600+ uses of error_state, for one thing) I think it would be a big step forward.
>
> Comments?  Objections?
>
> jwe