[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Using C++ exceptions instead of error_state
From: |
John W. Eaton |
Subject: |
Re: Using C++ exceptions instead of error_state |
Date: |
Mon, 22 Dec 2014 17:11:25 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.2.0 |
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