* Re: [no subject]
@ 2016-12-28 7:34 Chris Gregory
2016-12-28 7:42 ` Simplify internal_catch () Paul Eggert
0 siblings, 1 reply; 10+ messages in thread
From: Chris Gregory @ 2016-12-28 7:34 UTC (permalink / raw)
Oops meant to reply to `Simplify internal_catch()'.
Anyway here is another patch with the same thing.
--
Chris Gregory
diff --git a/src/eval.c b/src/eval.c
index e50e26a..edb41b6 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1337,20 +1337,12 @@ 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);
- if (sys_setjmp (c->jmp))
- {
- Lisp_Object val = handlerlist->val;
- clobbered_eassert (handlerlist == c);
- handlerlist = handlerlist->next;
- return hfun (val);
- }
- else
- {
- Lisp_Object val = bfun (arg);
- clobbered_eassert (handlerlist == c);
- handlerlist = handlerlist->next;
- return val;
- }
+ bool is_returning = sys_setjmp (c->jmp);
+ Lisp_Object val = is_returning ? handlerlist->val : bfun (arg);
+
+ clobbered_eassert (handlerlist == c);
+ handlerlist = handlerlist->next;
+ return is_returning ? hfun (val) : val;
}
/* Like internal_condition_case_1 but call BFUN with ARG1 and ARG2 as
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Simplify internal_catch ()
2016-12-28 7:34 [no subject] Chris Gregory
@ 2016-12-28 7:42 ` Paul Eggert
0 siblings, 0 replies; 10+ messages in thread
From: Paul Eggert @ 2016-12-28 7:42 UTC (permalink / raw)
To: Chris Gregory, emacs-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Simplify internal_catch()
@ 2016-12-28 6:33 Chris Gregory
2016-12-28 8:48 ` Andreas Schwab
2016-12-28 16:45 ` Stefan Monnier
0 siblings, 2 replies; 10+ messages in thread
From: Chris Gregory @ 2016-12-28 6:33 UTC (permalink / raw)
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Simplify internal_catch()
2016-12-28 6:33 Simplify internal_catch() Chris Gregory
@ 2016-12-28 8:48 ` Andreas Schwab
2016-12-28 16:45 ` Stefan Monnier
1 sibling, 0 replies; 10+ messages in thread
From: Andreas Schwab @ 2016-12-28 8:48 UTC (permalink / raw)
To: Chris Gregory; +Cc: monnier, emacs-devel
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."
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Simplify internal_catch()
2016-12-28 6:33 Simplify internal_catch() Chris Gregory
2016-12-28 8:48 ` Andreas Schwab
@ 2016-12-28 16:45 ` Stefan Monnier
2016-12-28 19:24 ` Paul Eggert
1 sibling, 1 reply; 10+ messages in thread
From: Stefan Monnier @ 2016-12-28 16:45 UTC (permalink / raw)
To: Chris Gregory; +Cc: emacs-devel
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Simplify internal_catch()
2016-12-28 16:45 ` Stefan Monnier
@ 2016-12-28 19:24 ` Paul Eggert
2016-12-28 19:28 ` Stefan Monnier
0 siblings, 1 reply; 10+ messages in thread
From: Paul Eggert @ 2016-12-28 19:24 UTC (permalink / raw)
To: Stefan Monnier, Chris Gregory; +Cc: emacs-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Simplify internal_catch()
2016-12-28 19:24 ` Paul Eggert
@ 2016-12-28 19:28 ` Stefan Monnier
2016-12-31 21:29 ` Paul Eggert
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Monnier @ 2016-12-28 19:28 UTC (permalink / raw)
To: Paul Eggert; +Cc: Chris Gregory, emacs-devel
>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Simplify internal_catch()
2016-12-28 19:28 ` Stefan Monnier
@ 2016-12-31 21:29 ` Paul Eggert
0 siblings, 0 replies; 10+ messages in thread
From: Paul Eggert @ 2016-12-31 21:29 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Chris Gregory, emacs-devel
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Simplify internal_catch()
@ 2016-12-28 1:54 Chris Gregory
2016-12-28 2:50 ` Stefan Monnier
0 siblings, 1 reply; 10+ messages in thread
From: Chris Gregory @ 2016-12-28 1:54 UTC (permalink / raw)
To: emacs-devel
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Simplify internal_catch()
2016-12-28 1:54 Chris Gregory
@ 2016-12-28 2:50 ` Stefan Monnier
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Monnier @ 2016-12-28 2:50 UTC (permalink / raw)
To: emacs-devel
> --- 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-12-31 21:29 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-28 7:34 [no subject] Chris Gregory
2016-12-28 7:42 ` Simplify internal_catch () Paul Eggert
-- strict thread matches above, loose matches on Subject: below --
2016-12-28 6:33 Simplify internal_catch() Chris Gregory
2016-12-28 8:48 ` Andreas Schwab
2016-12-28 16:45 ` Stefan Monnier
2016-12-28 19:24 ` Paul Eggert
2016-12-28 19:28 ` Stefan Monnier
2016-12-31 21:29 ` Paul Eggert
2016-12-28 1:54 Chris Gregory
2016-12-28 2:50 ` Stefan Monnier
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.