* Re: [PATCH] Remove NOERREXIT_UNTIL_EXEC
2022-12-03 0:31 [PATCH] Remove NOERREXIT_UNTIL_EXEC Philippe Altherr
@ 2022-12-03 0:34 ` Philippe Altherr
2022-12-04 5:28 ` Bart Schaefer
1 sibling, 0 replies; 3+ messages in thread
From: Philippe Altherr @ 2022-12-03 0:34 UTC (permalink / raw)
To: Zsh hackers list; +Cc: Bart Schaefer
[-- Attachment #1.1: Type: text/plain, Size: 3883 bytes --]
Stupid me. I forgot the patch! Here it is.
Philippe
On Sat, Dec 3, 2022 at 1:31 AM Philippe Altherr <philippe.altherr@gmail.com>
wrote:
> The noerrexit NOERREXIT_UNTIL_EXEC bit is never set and can therefore be
> removed. In other words the patch only removes dead code.
>
> I have been intrigued by the NOERREXIT_UNTIL_EXEC bit for a while. I
> didn't understand its purpose nor why the NOERREXIT_EXIT and
> NOERREXIT_RETURN bits wouldn't be enough to solve the problem at hand.
> After finally having a closer look, I must admit that I still don't
> understand. However, I discovered that the bit is never set and can safely
> be removed. More on that below.
>
> The bit was introduced in commit d6a32dd
> <https://github.com/zsh-users/zsh/commit/d6a32ddeed914434f5b56b013c9d03b28781d065>,
> where the noerrexit value 2 corresponds to today's NOERREXIT_UNTIL_EXEC and
> the value 1 to NOERREXIT_EXIT. There was no distinction yet between
> NOERREXIT_EXIT and NOERREXIT_RETURN. At that time, there was also no
> this_noerrexit variable. The distinction between NOERREXIT_EXIT and
> NOERREXIT_RETURN was introduced in commit 97d4bdb
> <https://github.com/zsh-users/zsh/commit/97d4bdbc7e86e6e8da0d4a059b118ffab289d3a9>,
> which also introduced today's noerrexit bits.
>
> One thing that distinguishes the NOERREXIT_UNTIL_EXEC bit from the other
> noerrexit bits is that it seems intended for the caller(s) of the function
> that sets it, while the other bits are intended for the callee(s). This is
> similar to today's this_noerrexit variable. I wonder whether this bit was a
> first attempt at solving the problem that is solved in today's code by the
> this_noerrexit variable. This would explain why everything is still fine
> even though the bit is no longer set.
>
> In today's code, the NOERREXIT_UNTIL_EXEC bit is only ever set in loop.c
> at line 578
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L578>.
> This line can only be reached if run != 0 && run !=2 && olderrexit == 0 &&
> lastval == 0. The loop above
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L551> can
> only be exited at one of the following lines:
>
> - line 551
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L551>=>
> run == 0
> - line 557
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L557>
> => run == 0 || run == 2
> - line 565
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L565>
> => run == 1 && lastval != 0
> - line 568
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L568>
> => run == 0 && lastval == 0
>
> None of these cases lead to line 578. Thus the NOERREXIT_UNTIL_EXEC bit is
> never set and can safely be removed.
>
> The code "noerrexit &= ~ (NOERREXIT_EXIT | NOERREXIT_RETURN)" at line 580
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/loop.c#L580> can
> be simplified into "noerrexit = olderrexit" because NOERREXIT_UNTIL_EXEC
> was the only noerrexit bit that was flowing from callees to their caller.
>
> Obviously all tests still pass. And I checked that the tests introduced by commit
> d6a32dd
> <https://github.com/zsh-users/zsh/commit/d6a32ddeed914434f5b56b013c9d03b28781d065> are
> still present.
>
> I have built this patch on top of my patch
> patch-06-fix-eval-and-source-statements.txt but I don't think that it
> depends on it nor on any of my previous patches (but I haven't tried to
> confirm it).
>
> QUESTION: Should NOERREXIT_SIGNAL's value
> <https://github.com/zsh-users/zsh/blob/41b402d36d0aeac594cf424a9e46b5edb20c815d/Src/zsh.h#L2226>
> be changed to 4 (from 8) since the value 4 is no longer used
> by NOERREXIT_UNTIL_EXEC?
>
> Philippe
>
>
[-- Attachment #1.2: Type: text/html, Size: 4841 bytes --]
[-- Attachment #2: patch-07-remove-noerrexit-until-exec.txt --]
[-- Type: text/plain, Size: 2165 bytes --]
diff --git a/Src/exec.c b/Src/exec.c
index 8ff6489ec..1dd569019 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1559,14 +1559,7 @@ execlist(Estate state, int dont_change_job, int exiting)
state->pc--;
sublist_done:
- /*
- * See hairy code near the end of execif() for the
- * following. "noerrexit " only applies until
- * we hit execcmd on the way down. We're now
- * on the way back up, so don't restore it.
- */
- if (!(oldnoerrexit & NOERREXIT_UNTIL_EXEC))
- noerrexit = oldnoerrexit;
+ noerrexit = oldnoerrexit;
if (sigtrapped[SIGDEBUG] && !isset(DEBUGBEFORECMD) && !donedebug) {
/*
@@ -3246,10 +3239,6 @@ execcmd_exec(Estate state, Execcmd_params eparams,
} else
preargs = NULL;
- /* if we get this far, it is OK to pay attention to lastval again */
- if (noerrexit & NOERREXIT_UNTIL_EXEC)
- noerrexit = 0;
-
/* Do prefork substitutions.
*
* Decide if we need "magic" handling of ~'s etc. in
diff --git a/Src/loop.c b/Src/loop.c
index 7c3e04b8a..61543ed73 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -569,23 +569,14 @@ execif(Estate state, int do_exec)
s = 1;
state->pc = next;
}
+ noerrexit = olderrexit;
if (run) {
- /* we need to ignore lastval until we reach execcmd() */
- if (olderrexit || run == 2)
- noerrexit = olderrexit;
- else if (lastval)
- noerrexit |= NOERREXIT_EXIT | NOERREXIT_RETURN | NOERREXIT_UNTIL_EXEC;
- else
- noerrexit &= ~ (NOERREXIT_EXIT | NOERREXIT_RETURN);
cmdpush(run == 2 ? CS_ELSE : (s ? CS_ELIFTHEN : CS_IFTHEN));
execlist(state, 1, do_exec);
cmdpop();
- } else {
- noerrexit = olderrexit;
- if (!retflag && !errflag)
- lastval = 0;
- }
+ } else if (!retflag && !errflag)
+ lastval = 0;
state->pc = end;
this_noerrexit = 1;
diff --git a/Src/zsh.h b/Src/zsh.h
index 40f9ea537..703231ca2 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2220,8 +2220,6 @@ enum noerrexit_bits {
NOERREXIT_EXIT = 1,
/* Suppress ERR_RETURN: per function call */
NOERREXIT_RETURN = 2,
- /* NOERREXIT only needed on way down */
- NOERREXIT_UNTIL_EXEC = 4,
/* Force exit on SIGINT */
NOERREXIT_SIGNAL = 8
};
^ permalink raw reply [flat|nested] 3+ messages in thread