Simplify internal_catch()

Simplify internal_catch()

[Chris Gregory]

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

This patch simplifies the structure of the internal_catch function.
If my change to prog_ignore is accepted (shown below), use the diff_with.  Otherwise
use diff_without.

THIS IS NOT THE DIFF FOR THIS PATCH!
diff --git a/src/eval.c b/src/eval.c
index e50e26a..abd4028 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -458,7 +458,11 @@ usage: (progn BODY...)  */)
 void
 prog_ignore (Lisp_Object body)
 {
-  Fprogn (body);
+  while (CONSP (body))
+    {
+      eval_sub (XCAR (body));
+      body = XCDR (body);
+    }
 }

THE DIFF FOR THIS PATCH is in the attachments.

-- 
Chris Gregory


[-- Attachment #2: diff_with --]
[-- Type: application/octet-stream, Size: 1146 bytes --]

diff --git a/src/eval.c b/src/eval.c
index abd4028..53e1f59 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1087,23 +1087,20 @@ internal_catch (Lisp_Object tag,
 		Lisp_Object (*func) (Lisp_Object), Lisp_Object arg)
 {
   /* This structure is made part of the chain `catchlist'.  */
-  struct handler *c = push_handler (tag, CATCHER);
+  struct handler *c;
+  Lisp_Object val;
+
+  c = push_handler (tag, CATCHER);
 
-  /* Call FUNC.  */
   if (! sys_setjmp (c->jmp))
-    {
-      Lisp_Object val = func (arg);
-      clobbered_eassert (handlerlist == c);
-      handlerlist = handlerlist->next;
-      return val;
-    }
+    /* Call FUNC.  */
+    val = func (arg);
   else
-    { /* Throw works by a longjmp that comes right here.  */
-      Lisp_Object val = handlerlist->val;
-      clobbered_eassert (handlerlist == c);
-      handlerlist = handlerlist->next;
-      return val;
-    }
+    /* Throw works by a longjmp that comes right here.  */
+    val = handlerlist->val;
+  clobbered_eassert (handlerlist == c);
+  handlerlist = handlerlist->next;
+  return val;
 }
 
 /* Unwind the specbind, catch, and handler stacks back to CATCH, and

[-- Attachment #3: diff_without --]
[-- Type: application/octet-stream, Size: 1146 bytes --]

diff --git a/src/eval.c b/src/eval.c
index e50e26a..29a479a 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1083,23 +1083,20 @@ internal_catch (Lisp_Object tag,
 		Lisp_Object (*func) (Lisp_Object), Lisp_Object arg)
 {
   /* This structure is made part of the chain `catchlist'.  */
-  struct handler *c = push_handler (tag, CATCHER);
+  struct handler *c;
+  Lisp_Object val;
+
+  c = push_handler (tag, CATCHER);
 
-  /* Call FUNC.  */
   if (! sys_setjmp (c->jmp))
-    {
-      Lisp_Object val = func (arg);
-      clobbered_eassert (handlerlist == c);
-      handlerlist = handlerlist->next;
-      return val;
-    }
+    /* Call FUNC.  */
+    val = func (arg);
   else
-    { /* Throw works by a longjmp that comes right here.  */
-      Lisp_Object val = handlerlist->val;
-      clobbered_eassert (handlerlist == c);
-      handlerlist = handlerlist->next;
-      return val;
-    }
+    /* Throw works by a longjmp that comes right here.  */
+    val = handlerlist->val;
+  clobbered_eassert (handlerlist == c);
+  handlerlist = handlerlist->next;
+  return val;
 }
 
 /* Unwind the specbind, catch, and handler stacks back to CATCH, and

Re: Simplify internal_catch()

[Stefan Monnier]

> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -458,7 +458,11 @@ usage: (progn BODY...)  */)
>  void
>  prog_ignore (Lisp_Object body)
>  {
> -  Fprogn (body);
> +  while (CONSP (body))
> +    {
> +      eval_sub (XCAR (body));
> +      body = XCDR (body);
> +    }
>  }

Why bother?  This is not performance-critical code, so the most
important criterion is to make the code clear and reduce redundancy.

> THE DIFF FOR THIS PATCH is in the attachments.

Both patches are identical, right?

> -  struct handler *c = push_handler (tag, CATCHER);
> +  struct handler *c;
> +  Lisp_Object val;
> +
> +  c = push_handler (tag, CATCHER);

Why place the declaration of `val` between the two?  I always strongly prefer
initializing a var directly in its declaration so that you simply cannot
refer to it in an uninitialized state.  Even more so for a variable
which is never modified afterwards.

>    if (! sys_setjmp (c->jmp))
> -    {
> -      Lisp_Object val = func (arg);
> -      clobbered_eassert (handlerlist == c);
> -      handlerlist = handlerlist->next;
> -      return val;
> -    }
> +    /* Call FUNC.  */
> +    val = func (arg);
>    else
> -    { /* Throw works by a longjmp that comes right here.  */
> -      Lisp_Object val = handlerlist->val;
> -      clobbered_eassert (handlerlist == c);
> -      handlerlist = handlerlist->next;
> -      return val;
> -    }
> +    /* Throw works by a longjmp that comes right here.  */
> +    val = handlerlist->val;
> +  clobbered_eassert (handlerlist == c);
> +  handlerlist = handlerlist->next;
> +  return val;

Right: sharing the tail is a good idea, thanks.


        Stefan

Re: Simplify internal_catch()

[Chris Gregory]

Here is the updated patch.  I replaced the if statement with a ternary
operator.

> Why place the declaration of `val` between the two?
I'm unsure as to the style desired for Emacs source code.  I've seen
both used in it.  I'll use the declaration initialization in the future
as I also prefer that.
-- 
Chris Gregory

diff --git a/src/eval.c b/src/eval.c
index e50e26a..54c09df 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1084,22 +1084,16 @@ internal_catch (Lisp_Object tag,
 {
   /* This structure is made part of the chain `catchlist'.  */
   struct handler *c = push_handler (tag, CATCHER);
-
-  /* Call FUNC.  */
-  if (! sys_setjmp (c->jmp))
-    {
-      Lisp_Object val = func (arg);
-      clobbered_eassert (handlerlist == c);
-      handlerlist = handlerlist->next;
-      return val;
-    }
-  else
-    { /* Throw works by a longjmp that comes right here.  */
-      Lisp_Object val = handlerlist->val;
-      clobbered_eassert (handlerlist == c);
-      handlerlist = handlerlist->next;
-      return val;
-    }
+  Lisp_Object val = (sys_setjmp (c->jmp)
+                     /* Call FUNC */
+                     ? func (arg)
+                     /* Throw works by a longjmp that comes here,
+                        setting this side */
+                     : handlerlist->val);
+
+  clobbered_eassert (handlerlist == c);
+  handlerlist = handlerlist->next;
+  return val;
 }
 
 /* Unwind the specbind, catch, and handler stacks back to CATCH, and

Re: Simplify internal_catch ()

[Paul Eggert]

Chris Gregory wrote:

> Anyway here is another patch with the same thing.

A few things. First, this particular patch won't do because functions like 
setjmp are special in C, and a declaration like "bool foo = setjmp (...);" is 
not portable (for details, please see section 7.13.1.1 of the C11 standard). 
Second, you're submitting so many proposed patches that for legal reasons we 
can't accept them or pay much attention to them without first getting copyright 
papers signed. (Please write to me privately if you and your employer are 
willing to do that.) Third, email proposing minor patches like this should 
normally be sent to bug-gnu-emacs@gnu.org instead of to emacs-devel@gnu.org. Thanks.

Re: Simplify internal_catch()

[Andreas Schwab]

On Dez 28 2016, Chris Gregory <czipperz@gmail.com> wrote:

> +  Lisp_Object val = (sys_setjmp (c->jmp)
> +                     /* Call FUNC */
> +                     ? func (arg)
> +                     /* Throw works by a longjmp that comes here,
> +                        setting this side */
> +                     : handlerlist->val);

setjmp is very special, it can only be used in the entire controlling
expression of a selection or iteration statement.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Simplify internal_catch()

[Stefan Monnier]

> I'm unsure as to the style desired for Emacs source code.  I've seen
> both used in it.

There's a fair bit of

    Lisp_Object foo;

    foo = ...;

around, indeed, because at some point in the past, some C compilers did
not support "Lisp_Object foo = ..." when Lisp_Object is a union type.
These issues are long gone, but the code remains ;-)


        Stefan

Re: Simplify internal_catch()

[Paul Eggert]

Stefan Monnier wrote:
> There's a fair bit of
>
>     Lisp_Object foo;
>
>     foo = ...;
>
> around, indeed, because at some point in the past, some C compilers did
> not support "Lisp_Object foo = ..." when Lisp_Object is a union type.

Actually, that kind of code was around because C89 did not allow declarations 
after statements. Even ancient C compilers allowed union-typed expressions as 
initial values for auto variables, and "Lisp_Object foo = ..." at block start 
has been in Emacs since the beginning; it's in the very oldest code (dated 1985) 
committed in the Emacs repository.

Re: Simplify internal_catch()

[Stefan Monnier]

>> There's a fair bit of
>> 
>> Lisp_Object foo;
>> 
>> foo = ...;
>> 
>> around, indeed, because at some point in the past, some C compilers did
>> not support "Lisp_Object foo = ..." when Lisp_Object is a union type.

> Actually, that kind of code was around because C89 did not allow
> declarations after statements.

In some cases, that's true.

> Even ancient C compilers allowed union-typed
> expressions as initial values for auto variables, and "Lisp_Object foo =
> ..." at block start has been in Emacs since the beginning; it's in the very
> oldest code (dated 1985) committed in the Emacs repository.

At least I know Richard made me undo some "Lisp_Object foo = ..." years
ago, saying that we shouldn't do that because it fails to compile in
some cases (presumably when you used some specific compiler and you also
asked for Lisp_Object to be a union rather than an int).
I did not heed his advice very eagerly, to tell you the truth.


        Stefan

Re: Simplify internal_catch()

[Paul Eggert]

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

Stefan Monnier wrote:
> At least I know Richard made me undo some "Lisp_Object foo = ..." years
> ago, saying that we shouldn't do that because it fails to compile in
> some cases (presumably when you used some specific compiler and you also
> asked for Lisp_Object to be a union rather than an int).
> I did not heed his advice very eagerly, to tell you the truth.

Many pre-C89 compilers indeed did not support union initializers (i.e., 'union u 
x = { something };', so perhaps there was some confusion about what worked and 
what didn't. Anyway, that water passed under the bridge long ago.

In reviewing this patch, I noticed some other confusion in this area that I 
introduced in 2013 while pacifying gcc -Wclobbered, and which I think you asked 
about but I didn't understand your question at the time (sorry). I attempted to 
fix the problem just now by installing the attached patch, which undoes Chris's 
simplification and then reworks the setjmp calls to use a local rather than a 
global. This avoid a need to load from a static variable in the normal flow of 
execution.

[-- Attachment #2: 0001-Clarify-internal_catch-etc.txt --]
[-- Type: text/plain, Size: 5526 bytes --]

From 535ef18ed523862db405d22ec4bea0bbfd4172ce Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 31 Dec 2016 13:10:38 -0800
Subject: [PATCH] Clarify internal_catch etc.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The recent change to internal_catch and friends relied on some
confusion I introduced to the code in 2013.  Attempt to fix
the confusion by clarifying the code instead.  This saves an
instruction and a load dependency in the typical case.
* src/eval.c (internal_catch, internal_condition_case)
(internal_condition_case_1, internal_condition_case_2)
(internal_condition_case_n): Undo the previous change.  Instead,
use use ‘c’ rather than ‘handlerlist’ in the typical case.
Also, use ‘eassert’ rather than ‘clobbered_eassert’ when possible.
---
 src/eval.c | 91 +++++++++++++++++++++++++++++---------------------------------
 1 file changed, 43 insertions(+), 48 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 01ae3a1..b174738 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1085,19 +1085,22 @@ internal_catch (Lisp_Object tag,
 {
   /* This structure is made part of the chain `catchlist'.  */
   struct handler *c = push_handler (tag, CATCHER);
-  Lisp_Object val;
 
   /* Call FUNC.  */
   if (! sys_setjmp (c->jmp))
-    val = func (arg);
-  else
     {
-      /* Throw works by a longjmp that comes right here.  */
-      val = handlerlist->val;
+      Lisp_Object val = func (arg);
+      eassert (handlerlist == c);
+      handlerlist = c->next;
+      return val;
+    }
+  else
+    { /* Throw works by a longjmp that comes right here.  */
+      Lisp_Object val = handlerlist->val;
+      clobbered_eassert (handlerlist == c);
+      handlerlist = handlerlist->next;
+      return val;
     }
-  clobbered_eassert (handlerlist == c);
-  handlerlist = handlerlist->next;
-  return val;
 }
 
 /* Unwind the specbind, catch, and handler stacks back to CATCH, and
@@ -1311,22 +1314,20 @@ internal_condition_case (Lisp_Object (*bfun) (void), Lisp_Object handlers,
 			 Lisp_Object (*hfun) (Lisp_Object))
 {
   struct handler *c = push_handler (handlers, CONDITION_CASE);
-  Lisp_Object val;
-  bool call_hfun;
-
   if (sys_setjmp (c->jmp))
     {
-      val = handlerlist->val;
-      call_hfun = true;
+      Lisp_Object val = handlerlist->val;
+      clobbered_eassert (handlerlist == c);
+      handlerlist = handlerlist->next;
+      return hfun (val);
     }
   else
     {
-      val = bfun ();
-      call_hfun = false;
+      Lisp_Object val = bfun ();
+      eassert (handlerlist == c);
+      handlerlist = c->next;
+      return val;
     }
-  clobbered_eassert (handlerlist == c);
-  handlerlist = handlerlist->next;
-  return call_hfun ? hfun (val) : val;
 }
 
 /* Like internal_condition_case but call BFUN with ARG as its argument.  */
@@ -1337,22 +1338,20 @@ internal_condition_case_1 (Lisp_Object (*bfun) (Lisp_Object), Lisp_Object arg,
 			   Lisp_Object (*hfun) (Lisp_Object))
 {
   struct handler *c = push_handler (handlers, CONDITION_CASE);
-  Lisp_Object val;
-  bool call_hfun;
-
   if (sys_setjmp (c->jmp))
     {
-      val = handlerlist->val;
-      call_hfun = true;
+      Lisp_Object val = handlerlist->val;
+      clobbered_eassert (handlerlist == c);
+      handlerlist = handlerlist->next;
+      return hfun (val);
     }
   else
     {
-      val = bfun (arg);
-      call_hfun = false;
+      Lisp_Object val = bfun (arg);
+      eassert (handlerlist == c);
+      handlerlist = c->next;
+      return val;
     }
-  clobbered_eassert (handlerlist == c);
-  handlerlist = handlerlist->next;
-  return call_hfun ? hfun (val) : val;
 }
 
 /* Like internal_condition_case_1 but call BFUN with ARG1 and ARG2 as
@@ -1366,22 +1365,20 @@ internal_condition_case_2 (Lisp_Object (*bfun) (Lisp_Object, Lisp_Object),
 			   Lisp_Object (*hfun) (Lisp_Object))
 {
   struct handler *c = push_handler (handlers, CONDITION_CASE);
-  Lisp_Object val;
-  bool call_hfun;
-
   if (sys_setjmp (c->jmp))
     {
-      val = handlerlist->val;
-      call_hfun = true;
+      Lisp_Object val = handlerlist->val;
+      clobbered_eassert (handlerlist == c);
+      handlerlist = handlerlist->next;
+      return hfun (val);
     }
   else
     {
-      val = bfun (arg1, arg2);
-      call_hfun = false;
+      Lisp_Object val = bfun (arg1, arg2);
+      eassert (handlerlist == c);
+      handlerlist = c->next;
+      return val;
     }
-  clobbered_eassert (handlerlist == c);
-  handlerlist = handlerlist->next;
-  return call_hfun ? hfun (val) : val;
 }
 
 /* Like internal_condition_case but call BFUN with NARGS as first,
@@ -1397,22 +1394,20 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
 						Lisp_Object *args))
 {
   struct handler *c = push_handler (handlers, CONDITION_CASE);
-  Lisp_Object val;
-  bool call_hfun;
-
   if (sys_setjmp (c->jmp))
     {
-      val = handlerlist->val;
-      call_hfun = true;
+      Lisp_Object val = handlerlist->val;
+      clobbered_eassert (handlerlist == c);
+      handlerlist = handlerlist->next;
+      return hfun (val, nargs, args);
     }
   else
     {
-      val = bfun (nargs, args);
-      call_hfun = false;
+      Lisp_Object val = bfun (nargs, args);
+      eassert (handlerlist == c);
+      handlerlist = c->next;
+      return val;
     }
-  clobbered_eassert (handlerlist == c);
-  handlerlist = handlerlist->next;
-  return call_hfun ? hfun (val, nargs, args) : val;
 }
 
 struct handler *
-- 
2.7.4