Short patch for review

Short patch for review

[Artur Malabarba]

Hi all, Kelly sent me a patch for changing a couple of `error'
instances into `user-error'.
It looks ok to me, but I'd appreciate if someone could confirm that
it's alright.


---
 src/ChangeLog  |   10 +++++++++-
 src/keyboard.c |    4 ++--
 src/lisp.h     |    1 +
 src/minibuf.c  |    2 +-
 4 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 76e1956..1c74a9a 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,6 +1,14 @@
+2015-02-18  Kelly Dean  <kelly@prtime.org>
+
+    * lisp.h:
+    * keyboard.c: Move declaration of user_error to header file.
+    * keyboard.c (recursive_edit_1): Signal Quser_error, not Qerror.
+    * minibuf.c (read_minibuf): Use user_error, not error.
+    (Bug#14480)
+
 2015-02-16  Kelly Dean  <kelly@prtime.org>

-    * src/keyboard.c (timer_check_2): Fix incorrect comment.
+    * keyboard.c (timer_check_2): Fix incorrect comment.

 2015-02-14  Martin Rudalics  <rudalics@gmx.at>

diff --git a/src/keyboard.c b/src/keyboard.c
index ac70062..304d8a2 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -731,7 +731,7 @@ recursive_edit_1 (void)
   /* Handle throw from read_minibuf when using minibuffer
      while it's active but we're in another window.  */
   if (STRINGP (val))
-    xsignal1 (Qerror, val);
+    xsignal1 (Quser_error, val);

   return unbind_to (count, Qnil);
 }
@@ -1179,7 +1179,7 @@ This also exits all active minibuffers.  */
   Fthrow (Qtop_level, Qnil);
 }

-static _Noreturn void
+_Noreturn void
 user_error (const char *msg)
 {
   xsignal1 (Quser_error, build_string (msg));
diff --git a/src/lisp.h b/src/lisp.h
index 7795c90..ca73d21 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4005,6 +4005,7 @@ extern Lisp_Object unbind_to (ptrdiff_t, Lisp_Object);
 extern _Noreturn void error (const char *, ...) ATTRIBUTE_FORMAT_PRINTF (1, 2);
 extern _Noreturn void verror (const char *, va_list)
   ATTRIBUTE_FORMAT_PRINTF (1, 0);
+extern _Noreturn void user_error (const char *);
 extern void un_autoload (Lisp_Object);
 extern Lisp_Object call_debugger (Lisp_Object arg);
 extern void init_eval_once (void);
diff --git a/src/minibuf.c b/src/minibuf.c
index 3408bb9..fca1a0d 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -449,7 +449,7 @@ read_minibuf (Lisp_Object map, Lisp_Object
initial, Lisp_Object prompt,
       && minibuf_level > 0)
     {
       if (EQ (selected_window, minibuf_window))
-    error ("Command attempted to use minibuffer while in minibuffer");
+    user_error ("Command attempted to use minibuffer while in minibuffer");
       else
     /* If we're in another window, cancel the minibuffer that's active.  */
     Fthrow (Qexit,
-- 
1.7.10.4

Re: Short patch for review

[Stefan Monnier]

> Hi all, Kelly sent me a patch for changing a couple of `error'
> instances into `user-error'.
> It looks ok to me, but I'd appreciate if someone could confirm that
> it's alright.

I agree with the intention.  I never know for sure how those _Noreturn
thingies work, so I can't guarantee that it does the right thing, but
the patch looks OK from a distance,


        Stefan


> ---
>  src/ChangeLog  |   10 +++++++++-
>  src/keyboard.c |    4 ++--
>  src/lisp.h     |    1 +
>  src/minibuf.c  |    2 +-
>  4 files changed, 13 insertions(+), 4 deletions(-)

> diff --git a/src/ChangeLog b/src/ChangeLog
> index 76e1956..1c74a9a 100644
> --- a/src/ChangeLog
> +++ b/src/ChangeLog
> @@ -1,6 +1,14 @@
> +2015-02-18  Kelly Dean  <kelly@prtime.org>
> +
> +    * lisp.h:
> +    * keyboard.c: Move declaration of user_error to header file.
> +    * keyboard.c (recursive_edit_1): Signal Quser_error, not Qerror.
> +    * minibuf.c (read_minibuf): Use user_error, not error.
> +    (Bug#14480)
> +
>  2015-02-16  Kelly Dean  <kelly@prtime.org>

> -    * src/keyboard.c (timer_check_2): Fix incorrect comment.
> +    * keyboard.c (timer_check_2): Fix incorrect comment.

>  2015-02-14  Martin Rudalics  <rudalics@gmx.at>

> diff --git a/src/keyboard.c b/src/keyboard.c
> index ac70062..304d8a2 100644
> --- a/src/keyboard.c
> +++ b/src/keyboard.c
> @@ -731,7 +731,7 @@ recursive_edit_1 (void)
>    /* Handle throw from read_minibuf when using minibuffer
>       while it's active but we're in another window.  */
>    if (STRINGP (val))
> -    xsignal1 (Qerror, val);
> +    xsignal1 (Quser_error, val);

>    return unbind_to (count, Qnil);
>  }
> @@ -1179,7 +1179,7 @@ This also exits all active minibuffers.  */
>    Fthrow (Qtop_level, Qnil);
>  }

> -static _Noreturn void
> +_Noreturn void
>  user_error (const char *msg)
>  {
>    xsignal1 (Quser_error, build_string (msg));
> diff --git a/src/lisp.h b/src/lisp.h
> index 7795c90..ca73d21 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -4005,6 +4005,7 @@ extern Lisp_Object unbind_to (ptrdiff_t, Lisp_Object);
>  extern _Noreturn void error (const char *, ...) ATTRIBUTE_FORMAT_PRINTF (1, 2);
>  extern _Noreturn void verror (const char *, va_list)
>    ATTRIBUTE_FORMAT_PRINTF (1, 0);
> +extern _Noreturn void user_error (const char *);
>  extern void un_autoload (Lisp_Object);
>  extern Lisp_Object call_debugger (Lisp_Object arg);
>  extern void init_eval_once (void);
> diff --git a/src/minibuf.c b/src/minibuf.c
> index 3408bb9..fca1a0d 100644
> --- a/src/minibuf.c
> +++ b/src/minibuf.c
> @@ -449,7 +449,7 @@ read_minibuf (Lisp_Object map, Lisp_Object
> initial, Lisp_Object prompt,
>        && minibuf_level > 0)
>      {
>        if (EQ (selected_window, minibuf_window))
> -    error ("Command attempted to use minibuffer while in minibuffer");
> +    user_error ("Command attempted to use minibuffer while in minibuffer");
>        else
>      /* If we're in another window, cancel the minibuffer that's active.  */
>      Fthrow (Qexit,
> -- 
> 1.7.10.4

Re: Short patch for review

[Eli Zaretskii]

> Date: Wed, 18 Feb 2015 19:17:52 -0200
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Cc: Kelly Dean <kelly@prtime.org>
> 
> Hi all, Kelly sent me a patch for changing a couple of `error'
> instances into `user-error'.
> It looks ok to me, but I'd appreciate if someone could confirm that
> it's alright.

Do we have guidelines for which errors should be 'user-error'?

Re: Short patch for review

[Kelly Dean]

Eli Zaretskii wrote:
> Do we have guidelines for which errors should be 'user-error'?

If the program is at fault, use error. If the user is at fault, use user-error.

If debug-on-error is t, and the program is at fault, then pop up the debugger in order to help find the problem in the program. If the user is at fault, then don't pop up the debugger, even if debug-on-error is t, because there's no problem in the program.

And those guidelines match, because Emacs pops up the debugger if debug-on-error is t and an error is signaled, but not if user-error is signaled.

In the case of this patch, the errors are clearly user errors, not program errors.

Re: Short patch for review

[Artur Malabarba]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

According to its docstring, it's for cases where the user does something
wrong. Calling a command on the wrong buffer, providing invalid input,
things like that.

We also have a few instances where it's used to abort commands when the
user answers "No" to a query. I'm not 100% sure those instances qualify,
but it's certainly way better than throwing actual errors.
On Feb 19, 2015 5:49 AM, "Eli Zaretskii" <eliz@gnu.org> wrote:

> > Date: Wed, 18 Feb 2015 19:17:52 -0200
> > From: Artur Malabarba <bruce.connor.am@gmail.com>
> > Cc: Kelly Dean <kelly@prtime.org>
> >
> > Hi all, Kelly sent me a patch for changing a couple of `error'
> > instances into `user-error'.
> > It looks ok to me, but I'd appreciate if someone could confirm that
> > it's alright.
>
> Do we have guidelines for which errors should be 'user-error'?
>

[-- Attachment #2: Type: text/html, Size: 1284 bytes --]

Re: Short patch for review

[Eli Zaretskii]

> Date: Thu, 19 Feb 2015 11:07:04 +0000
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Cc: emacs-devel <emacs-devel@gnu.org>, Kelly Dean <kelly@prtime.org>
> 
> According to its docstring, it's for cases where the user does something
> wrong. Calling a command on the wrong buffer, providing invalid input,
> things like that.

Can the entry to the minibuffer when one is already active happen only
by user action?

Re: Short patch for review

[Stefan Monnier]

> Do we have guidelines for which errors should be 'user-error'?

The idea is that if an error is normally not the result of an Elisp bug
but a "pilot error", we should use `user-error' so that users who set
debug-on-error to t don't get dropped into the debugger when they hit
this signal.

Now, in practice, I think most/all places where we signal an error can
sometimes be due to a pilot error and sometimes due to a bug in Elisp.
So it's a judgment call.

I wish we could have a more reliable way to handle that.
To the best of my knowledge we *could* do better by indicating for those
"user-error" a condition under which this should be treated as a "pilot
error", and this condition usually looks like "we're called from
function <foo> and that function was called interactively".

E.g. if forward-sexp bumps into EOB it's a user-error when forward-sexp
was called interactively, but it's a normal error otherwise.
But of course, if forward-sexp was called from a wrapper which itself
was called interactively it might still be a user-error.
IOW doing it right automatically is pretty much impossible.

So we're currently stuck with this judgment call.


        Stefan