[PATCH] internal: Add symbolic names to possible values of zexit()'s "from_where" parameter. No functional change.

[PATCH] internal: Add symbolic names to possible values of zexit()'s "from_where" parameter. No functional change.

[Daniel Shahaf]

---
Just a small update while I try to get my head around the exit-related codepaths.

Cheers,

Daniel


 Src/Modules/zpty.c |  2 +-
 Src/Zle/zle_main.c |  4 ++--
 Src/builtin.c      | 24 +++++++++++++-----------
 Src/exec.c         |  2 +-
 Src/init.c         | 10 +++++-----
 Src/signals.c      |  8 ++++----
 Src/subst.c        |  2 +-
 Src/zsh.h          |  8 ++++++++
 8 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c
index 2f83f7ce6..45fd15ee0 100644
--- a/Src/Modules/zpty.c
+++ b/Src/Modules/zpty.c
@@ -426,7 +426,7 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 	execode(prog, 1, 0, "zpty");
 	stopmsg = 2;
 	mypid = 0; /* trick to ensure we _exit() */
-	zexit(lastval, 0);
+	zexit(lastval, ZEXIT_NORMAL);
     }
     master = movefd(master);
     if (master == -1) {
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 27dc8ef21..22cb21be3 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -905,7 +905,7 @@ getbyte(long do_keytmout, int *timeout, int full)
 		if ((zlereadflags & ZLRF_IGNOREEOF) && icnt++ < 20)
 		    continue;
 		stopmsg = 1;
-		zexit(1, 0);
+		zexit(1, ZEXIT_NORMAL);
 	    }
 	    icnt = 0;
 	    if (errno == EINTR) {
@@ -928,7 +928,7 @@ getbyte(long do_keytmout, int *timeout, int full)
 	    } else if (errno != 0) {
 		zerr("error on TTY read: %e", errno);
 		stopmsg = 1;
-		zexit(1, 0);
+		zexit(1, ZEXIT_NORMAL);
 	    }
 	}
 	if (cc == '\r')		/* undo the exchange of \n and \r determined by */
diff --git a/Src/builtin.c b/Src/builtin.c
index 00b5d5c50..568b5acb9 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5665,7 +5665,7 @@ bin_break(char *name, char **argv, UNUSED(Options ops), int func)
 	    }
 	    return lastval;
 	}
-	zexit(num, 0);	/* else treat return as logout/exit */
+	zexit(num, ZEXIT_NORMAL);	/* else treat return as logout/exit */
 	break;
     case BIN_LOGOUT:
 	if (unset(LOGINSHELL)) {
@@ -5687,7 +5687,7 @@ bin_break(char *name, char **argv, UNUSED(Options ops), int func)
 	     * If we are already exiting... give this all up as
 	     * a bad job.
 	     */
-	    if (stopmsg || (zexit(0,2), !stopmsg)) {
+	    if (stopmsg || (zexit(0, ZEXIT_DEFERRED), !stopmsg)) {
 		retflag = 1;
 		breaks = loops;
 		exit_pending = 1;
@@ -5695,7 +5695,7 @@ bin_break(char *name, char **argv, UNUSED(Options ops), int func)
 		exit_val = num;
 	    }
 	} else
-	    zexit(num, 0);
+	    zexit(num, ZEXIT_NORMAL);
 	break;
     }
     return 0;
@@ -5780,14 +5780,15 @@ _realexit(void)
 
 /* exit the shell.  val is the return value of the shell.  *
  * from_where is
- *   1   if zexit is called because of a signal
- *   2   if we can't actually exit yet (e.g. functions need
- *       terminating) but should perform the usual interactive tests.
+ *   ZEXIT_SIGNAL   if zexit is called because of a signal
+ *   ZEXIT_DEFERRED if we can't actually exit yet (e.g. functions need
+ *                  terminating) but should perform the usual interactive
+ *                  tests.
  */
 
 /**/
 mod_export void
-zexit(int val, int from_where)
+zexit(int val, enum zexit_t from_where)
 {
     /*
      * Don't do anything recursively:  see below.
@@ -5798,7 +5799,7 @@ zexit(int val, int from_where)
     if (shell_exiting == -1)
 	return;
 
-    if (isset(MONITOR) && !stopmsg && from_where != 1) {
+    if (isset(MONITOR) && !stopmsg && from_where != ZEXIT_SIGNAL) {
 	scanjobs();    /* check if jobs need printing           */
 	if (isset(CHECKJOBS))
 	    checkjobs();   /* check if any jobs are running/stopped */
@@ -5808,7 +5809,8 @@ zexit(int val, int from_where)
 	}
     }
     /* Positive shell_exiting means we have been here before */
-    if (from_where == 2 || (shell_exiting++ && from_where))
+    if (from_where == ZEXIT_DEFERRED ||
+	(shell_exiting++ && from_where != ZEXIT_NORMAL))
 	return;
 
     /*
@@ -5824,12 +5826,12 @@ zexit(int val, int from_where)
 
     if (isset(MONITOR)) {
 	/* send SIGHUP to any jobs left running  */
-	killrunjobs(from_where == 1);
+	killrunjobs(from_where == ZEXIT_SIGNAL);
     }
     if (isset(RCS) && interact) {
 	if (!nohistsave) {
 	    int writeflags = HFILE_USE_OPTIONS;
-	    if (from_where == 1)
+	    if (from_where == ZEXIT_SIGNAL)
 		writeflags |= HFILE_NO_REWRITE;
 	    saveandpophiststack(1, writeflags);
 	    savehistfile(NULL, 1, writeflags);
diff --git a/Src/exec.c b/Src/exec.c
index 64eee7dc4..0d9d7de7c 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5949,7 +5949,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	     * exit command was handled.
 	     */
 	    stopmsg = 1;
-	    zexit(exit_val, 0);
+	    zexit(exit_val, ZEXIT_NORMAL);
 	}
     }
 
diff --git a/Src/init.c b/Src/init.c
index 2306d7bdf..2e2ef881c 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -161,7 +161,7 @@ loop(int toplevel, int justonce)
 		 * Handle that now.
 		 */
 		stopmsg = 1;
-		zexit(exit_val, 0);
+		zexit(exit_val, ZEXIT_NORMAL);
 	    }
 	    if (tok == LEXERR && !lastval)
 		lastval = 1;
@@ -1371,7 +1371,7 @@ init_misc(char *cmd, char *zsh_name)
 	bshin = fdopen(SHIN, "r");
 	execstring(cmd, 0, 1, "cmdarg");
 	stopmsg = 1;
-	zexit((exit_pending || shell_exiting) ? exit_val : lastval, 0);
+	zexit((exit_pending || shell_exiting) ? exit_val : lastval, ZEXIT_NORMAL);
     }
 
     if (interact && isset(RCS))
@@ -1778,20 +1778,20 @@ zsh_main(UNUSED(int argc), char **argv)
 	    if (!lastval)
 		lastval = 1;
 	    stopmsg = 1;
-	    zexit(lastval, 0);
+	    zexit(lastval, ZEXIT_NORMAL);
 	}
 	if (!(isset(IGNOREEOF) && interact)) {
 #if 0
 	    if (interact)
 		fputs(islogin ? "logout\n" : "exit\n", shout);
 #endif
-	    zexit(lastval, 0);
+	    zexit(lastval, ZEXIT_NORMAL);
 	    continue;
 	}
 	noexitct++;
 	if (noexitct >= 10) {
 	    stopmsg = 1;
-	    zexit(lastval, 0);
+	    zexit(lastval, ZEXIT_NORMAL);
 	}
 	/*
 	 * Don't print the message if it was already handled by
diff --git a/Src/signals.c b/Src/signals.c
index 14218177a..96ff9e9b3 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -654,7 +654,7 @@ zhandler(int sig)
 		_exit(SIGPIPE);
 	    else if (!isatty(SHTTY)) {
 		stopmsg = 1;
-		zexit(SIGPIPE, 1);
+		zexit(SIGPIPE, ZEXIT_SIGNAL);
 	    }
 	}
 	break;
@@ -662,7 +662,7 @@ zhandler(int sig)
     case SIGHUP:
         if (!handletrap(SIGHUP)) {
             stopmsg = 1;
-            zexit(SIGHUP, 1);
+            zexit(SIGHUP, ZEXIT_SIGNAL);
         }
         break;
  
@@ -670,7 +670,7 @@ zhandler(int sig)
         if (!handletrap(SIGINT)) {
 	    if ((isset(PRIVILEGED) || isset(RESTRICTED)) &&
 		isset(INTERACTIVE) && (noerrexit & NOERREXIT_SIGNAL))
-		zexit(SIGINT, 1);
+		zexit(SIGINT, ZEXIT_SIGNAL);
             if (list_pipe || chline || simple_pline) {
                 breaks = loops;
                 errflag |= ERRFLAG_INT;
@@ -703,7 +703,7 @@ zhandler(int sig)
 		errflag = noerrs = 0;
 		zwarn("timeout");
 		stopmsg = 1;
-		zexit(SIGALRM, 1);
+		zexit(SIGALRM, ZEXIT_SIGNAL);
 	    }
         }
         break;
diff --git a/Src/subst.c b/Src/subst.c
index f887dbd24..79efc9ad2 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -3044,7 +3044,7 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags,
                              * shouldn't be any if not interactive.
                              */
                             stopmsg = 1;
-                            zexit(1, 0);
+                            zexit(1, ZEXIT_NORMAL);
                         } else
                             _exit(1);
                     }
diff --git a/Src/zsh.h b/Src/zsh.h
index 9194ea82c..7b7aef540 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -3222,6 +3222,14 @@ enum {
 /* Hooks in core.                      */
 /***************************************/
 
+/* The type zexit()'s second parameter, which see. */
+enum zexit_t {
+    /* This isn't a bitfield. The values are here just for explicitness. */
+    ZEXIT_NORMAL = 0,
+    ZEXIT_SIGNAL = 1,
+    ZEXIT_DEFERRED = 2
+};
+
 #define EXITHOOK       (zshhooks + 0)
 #define BEFORETRAPHOOK (zshhooks + 1)
 #define AFTERTRAPHOOK  (zshhooks + 2)

Re: [PATCH] internal: Add symbolic names to possible values of zexit()'s "from_where" parameter. No functional change.

[dana]

On 16 Dec 2019, at 22:46, Daniel Shahaf <danielsh@apache.org> wrote:
> +/* The type zexit()'s second parameter, which see. */

Did some words get left out here?

dana

Re: [PATCH] internal: Add symbolic names to possible values of zexit()'s "from_where" parameter. No functional change.

[Daniel Shahaf]

dana wrote on Tue, Dec 17, 2019 at 01:21:19 -0600:
> On 16 Dec 2019, at 22:46, Daniel Shahaf <danielsh@apache.org> wrote:
> > +/* The type zexit()'s second parameter, which see. */
> 
> Did some words get left out here?

Yes, "of".  I had noticed this too and fixed it before pushing.  Thanks for the
review!

Re: [PATCH] internal: Add symbolic names to possible values of zexit()'s "from_where" parameter. No functional change.

[dana]

On 17 Dec 2019, at 01:32, Daniel Shahaf <danielsh@apache.org> wrote:
> Yes, "of".  I had noticed this too and fixed it before pushing.  Thanks for
> the review!

Oh, i didn't notice the push. I was more confused by the 'which see' part at
the end, though

dana

Re: [PATCH] internal: Add symbolic names to possible values of zexit()'s "from_where" parameter. No functional change.

[Daniel Shahaf]

dana wrote on Tue, 17 Dec 2019 07:41 +00:00:
> On 17 Dec 2019, at 01:32, Daniel Shahaf <danielsh@apache.org> wrote:
> > Yes, "of".  I had noticed this too and fixed it before pushing.  Thanks for
> > the review!
> 
> Oh, i didn't notice the push.

I get email notifications from sourceforge upon pushes.  There's
probably a way for you to get those too, if you want.

> I was more confused by the 'which see' part at
> the end, though

It's not a mistake.  "which" refers to "zexit()'s second parameter", and
is the direct object of the verb "see".  It's a set phrase — probably
began as a translation of <https://en.wiktionary.org/wiki/q.v.#Adverb>.
It basically means "Further details are in zexit()'s docstring.".

Cheers,

Daniel

Re: [PATCH] internal: Add symbolic names to possible values of zexit()'s "from_where" parameter. No functional change.

[dana]

On 17 Dec 2019, at 01:51, Daniel Shahaf <danielsh@apache.org> wrote:
> It's not a mistake.  "which" refers to "zexit()'s second parameter", and
> is the direct object of the verb "see".  It's a set phrase — probably
> began as a translation of <https://en.wiktionary.org/wiki/q.v.#Adverb>.

Oh, somehow i had never seen it written like that. Just my ignorance then,
sorry for the noise

dana

Re: [PATCH] internal: Add symbolic names to possible values of zexit()'s "from_where" parameter. No functional change.

[Mikael Magnusson]

On 12/17/19, dana <dana@dana.is> wrote:
> On 17 Dec 2019, at 01:51, Daniel Shahaf <danielsh@apache.org> wrote:
>> It's not a mistake.  "which" refers to "zexit()'s second parameter", and
>> is the direct object of the verb "see".  It's a set phrase — probably
>> began as a translation of <https://en.wiktionary.org/wiki/q.v.#Adverb>.
>
> Oh, somehow i had never seen it written like that. Just my ignorance then,
> sorry for the noise

I also have never seen it, and it seems to add no information to the
sentence either (I already know that following references is an
option). Remove to reduce confusion for readers?

-- 
Mikael Magnusson

Re: [PATCH] internal: Add symbolic names to possible values of zexit()'s "from_where" parameter. No functional change.

[Daniel Shahaf]

Mikael Magnusson wrote on Tue, 17 Dec 2019 09:46 +00:00:
> On 12/17/19, dana <dana@dana.is> wrote:
> > On 17 Dec 2019, at 01:51, Daniel Shahaf <danielsh@apache.org> wrote:
> >> It's not a mistake.  "which" refers to "zexit()'s second parameter", and
> >> is the direct object of the verb "see".  It's a set phrase — probably
> >> began as a translation of <https://en.wiktionary.org/wiki/q.v.#Adverb>.
> >
> > Oh, somehow i had never seen it written like that. Just my ignorance then,
> > sorry for the noise
> 
> I also have never seen it, and it seems to add no information to the
> sentence either (I already know that following references is an
> option). Remove to reduce confusion for readers?

I wrote it deliberately, since I felt that documenting an enum type as
"This is the type of the 2nd parameter to some_func()" wouldn't be best
practice either.  The "which see" addendum was intended to convey: "Yes,
this docstring doesn't tell you all of the information a docstring
is expected to convey; instead, that information is behind the
given pointer".

However, I don't feel strongly about it.  Whatever people prefer works for me.