improving error messages?

[Jaroslav Hajek-2]

hi,

I'm thinking about possible way to improve some error messages. The problem:
It is a good custom to prefix an error message with the name of the
originating function.
The problem is that some messages originate from procedures that have
no information about the invoking procedure.
In particular, this concerns functions from liboctave. Some of these
simply assume the calling function (e.g. sub2ind),
use something general (like "A(I):"), or use nothing (idx_vector).

cf. this:
octave:1> cellslices (1:3, 1, 4)
error: A(I): Index exceeds matrix dimension.

the problem is that the message doesn't show it originated in
"cellslices". This is primarily because Array<T>::index, which throws
it, doesn't know where it's being called from. Even though a traceback
will show the line number, if this occurs within a script, an user may
be easily confused.

Another instance of the same problem, within the interpreter, are
functions like octave_value::int_value, which gripe about "conversion
failed", but don't show the calling function.
Basically, there are three existing approaches currently in use to
face the problem:

1. ignore it
2. check for the error and display a supplemental message (on a new
line) with a correct prefix
3. try to pass the name down the call chain

1 is done for liboctave, as explained. 2 is used by some functions, e.g.
octave:1> any(1, 1.5)
error: conversion of 1.5 to int value failed
error: any: expecting dimension argument to be an integer

the problem is that this duplicates some information and requires a
lot of checking.
Approach 3 is used by some of the gripe_xxx functions. Still, it is
not reasonable to pass the name everywhere, especially to liboctave.

Looking for a relatively simple solution, I thought that error handler
simply can optionally query the current function name and insert it
into the message.
Since a lot of functions already do it in the first place, it can
first check whether the prefix is already there.

I ended up with the attached patch. This seems to mostly solve my problems:

octave:1> cellslices (1:3, 1, 4)
error: cellslices: A(I): Index exceeds matrix dimension.
octave:1> any(1, 1.5)
error: any: conversion of 1.5 to int value failed
error: any: expecting dimension argument to be an integer

I know this is no candidate for the best software design award, but it
works and is a relatively harmless change...

Comments?

--
RNDr. Jaroslav Hajek
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz

[errors.diff]

---

# HG changeset patch
# User Jaroslav Hajek <highegg@...>
# Date 1256121474 -7200
# Node ID 32745d8e700130788cb7ca702a2c686e8d4b5984
# Parent  7bda650b691a21434cddb2452a0fbc77c558a9ca
[mq]: errors.diff

diff --git a/src/error.cc b/src/error.cc
--- a/src/error.cc
+++ b/src/error.cc
@@ -204,7 +204,8 @@
 
 static void
 verror (bool save_last_error, std::ostream& os,
- const char *name, const char *id, const char *fmt, va_list args)
+ const char *name, const char *id, const char *fmt, va_list args,
+        bool with_cfn = false)
 {
   if (discard_error_messages)
     return;
@@ -232,6 +233,24 @@
   if (name)
     msg_string += std::string (name) + ": ";
 
+  if (with_cfn)
+    {
+      octave_function *curfcn = octave_call_stack::current ();
+      if (curfcn)
+        {
+          std::string cfn = curfcn->name ();
+          if (! cfn.empty ())
+            {
+              cfn += ':';
+              if (cfn.length () > base_msg.length ()
+                 || base_msg.compare (0, cfn.length (), cfn) != 0)
+                {
+                  msg_string += cfn + ' ';
+                }
+            }
+        }
+    }
+
   msg_string += base_msg + "\n";
 
   if (! error_state && save_last_error)
@@ -275,7 +294,7 @@
 
 static void
 error_1 (std::ostream& os, const char *name, const char *id,
- const char *fmt, va_list args)
+ const char *fmt, va_list args, bool with_cfn = false)
 {
   if (error_state != -2)
     {
@@ -293,7 +312,7 @@
  {
   char *tmp_fmt = strsave (fmt);
   tmp_fmt[len - 1] = '\0';
-  verror (true, os, name, id, tmp_fmt, args);
+  verror (true, os, name, id, tmp_fmt, args, with_cfn);
   delete [] tmp_fmt;
  }
 
@@ -301,7 +320,7 @@
     }
   else
     {
-      verror (true, os, name, id, fmt, args);
+      verror (true, os, name, id, fmt, args, with_cfn);
 
       if (! error_state)
  error_state = 1;
@@ -453,11 +472,11 @@
 }
 
 static void
-error_2 (const char *id, const char *fmt, va_list args)
+error_2 (const char *id, const char *fmt, va_list args, bool with_cfn = false)
 {
   int init_state = error_state;
 
-  error_1 (std::cerr, "error", id, fmt, args);
+  error_1 (std::cerr, "error", id, fmt, args, with_cfn);
 
   if ((interactive || forced_interactive)
       && Vdebug_on_error && init_state == 0
@@ -492,6 +511,21 @@
 }
 
 void
+verror_with_cfn (const char *fmt, va_list args)
+{
+  error_2 ("", fmt, args, true);
+}
+
+void
+error_with_cfn (const char *fmt, ...)
+{
+  va_list args;
+  va_start (args, fmt);
+  verror_with_cfn (fmt, args);
+  va_end (args);
+}
+
+void
 verror_with_id (const char *id, const char *fmt, va_list args)
 {
   error_2 (id, fmt, args);
@@ -506,6 +540,21 @@
   va_end (args);
 }
 
+void
+verror_with_id_cfn (const char *id, const char *fmt, va_list args)
+{
+  error_2 (id, fmt, args, true);
+}
+
+void
+error_with_id_cfn (const char *id, const char *fmt, ...)
+{
+  va_list args;
+  va_start (args, fmt);
+  verror_with_id_cfn (id, fmt, args);
+  va_end (args);
+}
+
 static int
 check_state (const std::string& state)
 {
diff --git a/src/error.h b/src/error.h
--- a/src/error.h
+++ b/src/error.h
@@ -47,6 +47,9 @@
 extern OCTINTERP_API void verror (const char *fmt, va_list args);
 extern OCTINTERP_API void error (const char *fmt, ...);
 
+extern OCTINTERP_API void verror_with_cfn (const char *fmt, va_list args);
+extern OCTINTERP_API void error_with_cfn (const char *fmt, ...);
+
 extern OCTINTERP_API void vparse_error (const char *fmt, va_list args);
 extern OCTINTERP_API void parse_error (const char *fmt, ...);
 
@@ -75,6 +78,12 @@
 error_with_id (const char *id, const char *fmt, ...);
 
 extern OCTINTERP_API void
+verror_with_id_cfn (const char *id, const char *fmt, va_list args);
+
+extern OCTINTERP_API void
+error_with_id_cfn (const char *id, const char *fmt, ...);
+
+extern OCTINTERP_API void
 vparse_error_with_id (const char *id, const char *fmt, va_list args);
 
 extern OCTINTERP_API void
diff --git a/src/octave.cc b/src/octave.cc
--- a/src/octave.cc
+++ b/src/octave.cc
@@ -552,7 +552,7 @@
 {
   va_list args;
   va_start (args, fmt);
-  verror (fmt, args);
+  verror_with_cfn (fmt, args);
   va_end (args);
 
   octave_throw_execution_exception ();
diff --git a/src/ov-base.cc b/src/ov-base.cc
--- a/src/ov-base.cc
+++ b/src/ov-base.cc
@@ -395,7 +395,7 @@
     if (! error_state) \
       { \
  if (require_int && D_NINT (d) != d) \
-  error ("conversion of %g to " #T " value failed", d); \
+  error_with_cfn ("conversion of %g to " #T " value failed", d); \
  else if (d < MIN_LIMIT) \
   retval = MIN_LIMIT; \
  else if (d > MAX_LIMIT) \
diff --git a/src/ov.cc b/src/ov.cc
--- a/src/ov.cc
+++ b/src/ov.cc
@@ -1594,7 +1594,7 @@
                     retval.xelem (i) = v;
                   else
                     {
-                      error ("conversion to integer value failed");
+                      error_with_cfn ("conversion to integer value failed");
                       break;
                     }
                 }
@@ -1676,7 +1676,7 @@
                     retval.xelem (i) = v;
                   else
                     {
-                      error ("conversion to integer value failed");
+                      error_with_cfn ("conversion to integer value failed");
                       break;
                     }
                 }

---

[Søren Hauberg]

ons, 21 10 2009 kl. 12:38 +0200, skrev Jaroslav Hajek:

> I ended up with the attached patch. This seems to mostly solve my problems:
>
> octave:1> cellslices (1:3, 1, 4)
> error: cellslices: A(I): Index exceeds matrix dimension.
> octave:1> any(1, 1.5)
> error: any: conversion of 1.5 to int value failed
> error: any: expecting dimension argument to be an integer
>
> I know this is no candidate for the best software design award, but it
> works and is a relatively harmless change...
>
> Comments?

Funny! I was thinking about the same problem just a few days ago, and I
arrived at the same solution. One question, though:

Let's say I'm developing a function 'A' that calls a bunch of other
functions, i.e.

  function A ()
    ...
    B ();
    ...
  endfunction

what then happens if 'B' raises an error? Will I then get an error
message from 'A' instead? This could potentially make it harder to
develop functions or am I missing something?

Søren

[Jaroslav Hajek-2]

On Wed, Oct 21, 2009 at 12:48 PM, Søren Hauberg <soren@...> wrote:

> ons, 21 10 2009 kl. 12:38 +0200, skrev Jaroslav Hajek:
>> I ended up with the attached patch. This seems to mostly solve my problems:
>>
>> octave:1> cellslices (1:3, 1, 4)
>> error: cellslices: A(I): Index exceeds matrix dimension.
>> octave:1> any(1, 1.5)
>> error: any: conversion of 1.5 to int value failed
>> error: any: expecting dimension argument to be an integer
>>
>> I know this is no candidate for the best software design award, but it
>> works and is a relatively harmless change...
>>
>> Comments?
>
> Funny! I was thinking about the same problem just a few days ago, and I
> arrived at the same solution. One question, though:
>
> Let's say I'm developing a function 'A' that calls a bunch of other
> functions, i.e.
>
>  function A ()
>    ...
>    B ();
>    ...
>  endfunction
>
> what then happens if 'B' raises an error? Will I then get an error
> message from 'A' instead? This could potentially make it harder to
> develop functions or am I missing something?
>
> Søren
>

No, you'll see a message from the innermost function that raised the
error, be it B or something else. And it only applies to errors
originating from liboctave exceptions (which generally have no way of
knowing who called them) or those generated by the C++ function
error_with_cfn (or error_with_id_cfn). So most user-generated errors
are unaffected.

--
RNDr. Jaroslav Hajek
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz

[Søren Hauberg]

ons, 21 10 2009 kl. 12:57 +0200, skrev Jaroslav Hajek:
> No, you'll see a message from the innermost function that raised the
> error, be it B or something else. And it only applies to errors
> originating from liboctave exceptions (which generally have no way of
> knowing who called them) or those generated by the C++ function
> error_with_cfn (or error_with_id_cfn). So most user-generated errors
> are unaffected.

Okay, that makes sense.

Thanks
Søren

[John W. Eaton-3]

On 21-Oct-2009, Jaroslav Hajek wrote:

| On Wed, Oct 21, 2009 at 12:48 PM, Søren Hauberg <soren@...> wrote:
| > ons, 21 10 2009 kl. 12:38 +0200, skrev Jaroslav Hajek:
| >> I ended up with the attached patch. This seems to mostly solve my problems:
| >>
| >> octave:1> cellslices (1:3, 1, 4)
| >> error: cellslices: A(I): Index exceeds matrix dimension.
| >> octave:1> any(1, 1.5)
| >> error: any: conversion of 1.5 to int value failed
| >> error: any: expecting dimension argument to be an integer
| >>
| >> I know this is no candidate for the best software design award, but it
| >> works and is a relatively harmless change...
| >>
| >> Comments?
| >
| > Funny! I was thinking about the same problem just a few days ago, and I
| > arrived at the same solution. One question, though:
| >
| > Let's say I'm developing a function 'A' that calls a bunch of other
| > functions, i.e.
| >
| >  function A ()
| >    ...
| >    B ();
| >    ...
| >  endfunction
| >
| > what then happens if 'B' raises an error? Will I then get an error
| > message from 'A' instead? This could potentially make it harder to
| > develop functions or am I missing something?
| >
| > Søren
| >
|
| No, you'll see a message from the innermost function that raised the
| error, be it B or something else. And it only applies to errors
| originating from liboctave exceptions (which generally have no way of
| knowing who called them) or those generated by the C++ function
| error_with_cfn (or error_with_id_cfn). So most user-generated errors
| are unaffected.

I think I'd rather not have to remember when it is appropriate to call
error_with_cfn.  Maybe it would be better if we changed things so that
the error function always reported the calling function information,
and then changed our style to not include the function name as an
error message prefix?

jwe

[Jaroslav Hajek-2]

On Wed, Oct 21, 2009 at 1:20 PM, John W. Eaton <jwe@...> wrote:

> On 21-Oct-2009, Jaroslav Hajek wrote:
>
> | On Wed, Oct 21, 2009 at 12:48 PM, Søren Hauberg <soren@...> wrote:
> | > ons, 21 10 2009 kl. 12:38 +0200, skrev Jaroslav Hajek:
> | >> I ended up with the attached patch. This seems to mostly solve my problems:
> | >>
> | >> octave:1> cellslices (1:3, 1, 4)
> | >> error: cellslices: A(I): Index exceeds matrix dimension.
> | >> octave:1> any(1, 1.5)
> | >> error: any: conversion of 1.5 to int value failed
> | >> error: any: expecting dimension argument to be an integer
> | >>
> | >> I know this is no candidate for the best software design award, but it
> | >> works and is a relatively harmless change...
> | >>
> | >> Comments?
> | >
> | > Funny! I was thinking about the same problem just a few days ago, and I
> | > arrived at the same solution. One question, though:
> | >
> | > Let's say I'm developing a function 'A' that calls a bunch of other
> | > functions, i.e.
> | >
> | >  function A ()
> | >    ...
> | >    B ();
> | >    ...
> | >  endfunction
> | >
> | > what then happens if 'B' raises an error? Will I then get an error
> | > message from 'A' instead? This could potentially make it harder to
> | > develop functions or am I missing something?
> | >
> | > Søren
> | >
> |
> | No, you'll see a message from the innermost function that raised the
> | error, be it B or something else. And it only applies to errors
> | originating from liboctave exceptions (which generally have no way of
> | knowing who called them) or those generated by the C++ function
> | error_with_cfn (or error_with_id_cfn). So most user-generated errors
> | are unaffected.
>
> I think I'd rather not have to remember when it is appropriate to call
> error_with_cfn.

It's simple: if you want the auto-prepending. That is, almost never in
normal code. For liboctave exceptions, it's done automatically in the
handler; otherwise, you want to call it in places which want to throw
a generic error for multiple built-in functions, such as various
octave_value::XXX_value extractors.

> Maybe it would be better if we changed things so that
> the error function always reported the calling function information,
> and then changed our style to not include the function name as an
> error message prefix?
>

Better, maybe. The point is that code gets more easily changed than
style :) Besides, doing that would probably require massive changes
all over the place. The final thing is that we would have to make it
quite smart; often the user wants to ignore helper or internal
functions that actually invoke the error, without the need to catch,
subst the prefix and rethrow...

--
RNDr. Jaroslav Hajek
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz

[Michael D. Godfrey]

On 10/21/2009 03:38 AM, Jaroslav Hajek wrote:

I know this is no candidate for the best software design award, but it
works and is a relatively harmless change...

Comments?
  

Major improvement.  Forget the design award. It works much
better than the present code.

Thanks!