|
From: | Rik |
Subject: | Re: Using C++ exceptions instead of error_state |
Date: | Mon, 22 Dec 2014 09:05:27 -0800 |
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 |
[Prev in Thread] | Current Thread | [Next in Thread] |