* [PATCH 2/2] Fix two C nits @ 2018-06-16 1:04 ` Eitan Adler 2018-06-18 9:22 ` Peter Stephenson 0 siblings, 1 reply; 7+ messages in thread From: Eitan Adler @ 2018-06-16 1:04 UTC (permalink / raw) To: zsh-workers; +Cc: Eitan Adler - avoid returning from a function that will never return - use "ifdef" instead of "if" for checking if preprocessor exists Signed-off-by: Eitan Adler <lists@eitanadler.com> --- Src/exec.c | 1 - Src/zsh_system.h | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/Src/exec.c b/Src/exec.c index d44527841..b36bcef64 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -4954,7 +4954,6 @@ getpipe(char *cmd, int nullexec) execode(prog, 0, 1, out ? "outsubst" : "insubst"); cmdpop(); _exit(lastval); - return 0; } /* open pipes with fds >= 10 */ diff --git a/Src/zsh_system.h b/Src/zsh_system.h index 5339b496f..d9b8617c5 100644 --- a/Src/zsh_system.h +++ b/Src/zsh_system.h @@ -735,7 +735,7 @@ extern char **environ; /* These variables are sometimes defined in, * * and needed by, the termcap library. */ -#if MUST_DEFINE_OSPEED +#ifdef MUST_DEFINE_OSPEED extern char PC, *BC, *UP; extern short ospeed; #endif -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Fix two C nits 2018-06-16 1:04 ` [PATCH 2/2] Fix two C nits Eitan Adler @ 2018-06-18 9:22 ` Peter Stephenson 2018-06-19 5:31 ` Eitan Adler 2018-06-19 13:46 ` Vincent Lefevre 0 siblings, 2 replies; 7+ messages in thread From: Peter Stephenson @ 2018-06-18 9:22 UTC (permalink / raw) To: zsh-workers On Sat, 16 Jun 2018 01:04:27 +0000 Eitan Adler <lists@eitanadler.com> wrote: > - avoid returning from a function that will never return > > diff --git a/Src/exec.c b/Src/exec.c > index d44527841..b36bcef64 100644 > --- a/Src/exec.c > +++ b/Src/exec.c > @@ -4954,7 +4954,6 @@ getpipe(char *cmd, int nullexec) > execode(prog, 0, 1, out ? "outsubst" : "insubst"); > cmdpop(); > _exit(lastval); > - return 0; > } I'm not 100% sure about this since you're relying on the compiler knowing that _exit won't return. Probably the majority of compilers we're involved with for zsh will work that out, but I'm not sure it's actually required by the C standard that they know the function doesn't return, is it? You may know some corner I haven't investigated. I'd have been tempted to add /*NOTREACHED*/ on the next line. pws ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Fix two C nits 2018-06-18 9:22 ` Peter Stephenson @ 2018-06-19 5:31 ` Eitan Adler 2018-06-19 13:46 ` Vincent Lefevre 1 sibling, 0 replies; 7+ messages in thread From: Eitan Adler @ 2018-06-19 5:31 UTC (permalink / raw) To: Peter Stephenson; +Cc: Zsh hackers list On 18 June 2018 at 02:22, Peter Stephenson <p.stephenson@samsung.com> wrote: > On Sat, 16 Jun 2018 01:04:27 +0000 > Eitan Adler <lists@eitanadler.com> wrote: >> - avoid returning from a function that will never return >> >> diff --git a/Src/exec.c b/Src/exec.c >> index d44527841..b36bcef64 100644 >> --- a/Src/exec.c >> +++ b/Src/exec.c >> @@ -4954,7 +4954,6 @@ getpipe(char *cmd, int nullexec) >> execode(prog, 0, 1, out ? "outsubst" : "insubst"); >> cmdpop(); >> _exit(lastval); >> - return 0; >> } > > I'm not 100% sure about this since you're relying on the compiler > knowing that _exit won't return. Probably the majority of compilers > we're involved with for zsh will work that out, but I'm not sure > it's actually required by the C standard that they know the function > doesn't return, is it? You may know some corner I haven't investigated. It is only required that any function either return, exit, or loop forever [0]. While I don't much about non-modern compilers, it shouldn't affect anything to remove the return. At best, it'll do nothing. At worst older compilers might incorrectly issue a diagnostic. IMHO we should defer to modern compilers when it comes to diagnostics provided the more esoteric options still work. > I'd have been tempted to add /*NOTREACHED*/ on the next line. [0] there is some interesting ancient nuance here, but it's not important for this discussion. -- Eitan Adler ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Fix two C nits 2018-06-18 9:22 ` Peter Stephenson 2018-06-19 5:31 ` Eitan Adler @ 2018-06-19 13:46 ` Vincent Lefevre 2018-06-19 14:13 ` Peter Stephenson 1 sibling, 1 reply; 7+ messages in thread From: Vincent Lefevre @ 2018-06-19 13:46 UTC (permalink / raw) To: zsh-workers On 2018-06-18 10:22:41 +0100, Peter Stephenson wrote: > On Sat, 16 Jun 2018 01:04:27 +0000 > Eitan Adler <lists@eitanadler.com> wrote: > > - avoid returning from a function that will never return > > > > diff --git a/Src/exec.c b/Src/exec.c > > index d44527841..b36bcef64 100644 > > --- a/Src/exec.c > > +++ b/Src/exec.c > > @@ -4954,7 +4954,6 @@ getpipe(char *cmd, int nullexec) > > execode(prog, 0, 1, out ? "outsubst" : "insubst"); > > cmdpop(); > > _exit(lastval); > > - return 0; > > } > > I'm not 100% sure about this since you're relying on the compiler > knowing that _exit won't return. Probably the majority of compilers > we're involved with for zsh will work that out, but I'm not sure > it's actually required by the C standard that they know the function > doesn't return, is it? You may know some corner I haven't investigated. AFAIK, it's only useful for optimization and optional diagnostics. The _exit function is non-standard. So, it depends on whether it is declared as _Noreturn or not. FYI, the standard C11 function is: _Noreturn void _Exit(int status); Perhaps use it, and _exit as a fallback. -- Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/> 100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Fix two C nits 2018-06-19 13:46 ` Vincent Lefevre @ 2018-06-19 14:13 ` Peter Stephenson 2018-06-19 14:38 ` Vincent Lefevre 0 siblings, 1 reply; 7+ messages in thread From: Peter Stephenson @ 2018-06-19 14:13 UTC (permalink / raw) To: zsh-workers On Tue, 19 Jun 2018 15:46:29 +0200 Vincent Lefevre <vincent@vinc17.net> wrote: > On 2018-06-18 10:22:41 +0100, Peter Stephenson wrote: > > On Sat, 16 Jun 2018 01:04:27 +0000 > > Eitan Adler <lists@eitanadler.com> wrote: > > > - avoid returning from a function that will never return > > > > > > diff --git a/Src/exec.c b/Src/exec.c > > > index d44527841..b36bcef64 100644 > > > --- a/Src/exec.c > > > +++ b/Src/exec.c > > > @@ -4954,7 +4954,6 @@ getpipe(char *cmd, int nullexec) > > > execode(prog, 0, 1, out ? "outsubst" : "insubst"); > > > cmdpop(); > > > _exit(lastval); > > > - return 0; > > > } > > The _exit function is non-standard. That means from the basic C point of view it's not safe simply to remove the return. We are not in the game of guessing what the compiler knows. As the code's uambiguously correct as it is, and the compiler is perfectly at liberty to optimise out the return if it knows the function won't return, it can stay as it is. Anything we *can* detect about the compiler / configuration is fair game for improvement, however... pws ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Fix two C nits 2018-06-19 14:13 ` Peter Stephenson @ 2018-06-19 14:38 ` Vincent Lefevre 2018-06-19 15:59 ` Peter Stephenson 0 siblings, 1 reply; 7+ messages in thread From: Vincent Lefevre @ 2018-06-19 14:38 UTC (permalink / raw) To: zsh-workers On 2018-06-19 15:13:19 +0100, Peter Stephenson wrote: > On Tue, 19 Jun 2018 15:46:29 +0200 > Vincent Lefevre <vincent@vinc17.net> wrote: > > On 2018-06-18 10:22:41 +0100, Peter Stephenson wrote: > > > On Sat, 16 Jun 2018 01:04:27 +0000 > > > Eitan Adler <lists@eitanadler.com> wrote: > > > > - avoid returning from a function that will never return > > > > > > > > diff --git a/Src/exec.c b/Src/exec.c > > > > index d44527841..b36bcef64 100644 > > > > --- a/Src/exec.c > > > > +++ b/Src/exec.c > > > > @@ -4954,7 +4954,6 @@ getpipe(char *cmd, int nullexec) > > > > execode(prog, 0, 1, out ? "outsubst" : "insubst"); > > > > cmdpop(); > > > > _exit(lastval); > > > > - return 0; > > > > } > > > > The _exit function is non-standard. > > That means from the basic C point of view it's not safe simply to remove > the return. We are not in the game of guessing what the compiler knows. It's safe because whether the "return 0;" line is here or not, this will not change the behavior since this line is not reachable (even if the compiler doesn't know this). If the compiler doesn't know that _exit never returns, it will typically add an instruction corresponding to the "return 0;", but this instruction will never be executed. If the "return 0;" is removed, then the compiler will not add such an instruction, but may add a "jump" (which, again, will never be executed) and this may also prevent some optimizations in the code that follows (because the compiler would think that code that follows may also be reached from this "if" block). In short, the presence or not of the "return 0;" may have an effect on the generated code (good or bad, this is potentially unknown), but the behavior will remain the same. > Anything we *can* detect about the compiler / configuration is > fair game for improvement, however... FYI, _exit is a builtin in GCC (according to the manual), so that GCC necessarily knows and the presence or not of "return 0;" should be equivalent. -- Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/> 100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Fix two C nits 2018-06-19 14:38 ` Vincent Lefevre @ 2018-06-19 15:59 ` Peter Stephenson 0 siblings, 0 replies; 7+ messages in thread From: Peter Stephenson @ 2018-06-19 15:59 UTC (permalink / raw) To: zsh-workers On Tue, 19 Jun 2018 16:38:34 +0200 Vincent Lefevre <vincent@vinc17.net> wrote: > It's safe because whether the "return 0;" line is here or not, this > will not change the behavior since this line is not reachable (even > if the compiler doesn't know this). > > If the compiler doesn't know that _exit never returns, it will > typically add an instruction corresponding to the "return 0;", If I were a compiler and I saw static int getpipe(char *cmd, int nullexec) { /* ... blah ... */ some_random_function(); } I would immediately warn about missing return values --- indeed, gcc does this with functions it hasn't been told about as I tested: gcc_return.c:14:1: warning: no return statement in function returning non-void [-Wreturn-type] so we've taken a step back in terms of clean code (even if gcc itself isn't the problem on a typical GNU-based system where it does know about _exit). Anyway, I've completely lost interest in this non-issue with what's currently entirely valid code and don't propose to discuss it further. pws ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-06-19 15:59 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20180616021439epcas4p327f4346b40d9e10d94ce055058fe0d71@epcas4p3.samsung.com> 2018-06-16 1:04 ` [PATCH 2/2] Fix two C nits Eitan Adler 2018-06-18 9:22 ` Peter Stephenson 2018-06-19 5:31 ` Eitan Adler 2018-06-19 13:46 ` Vincent Lefevre 2018-06-19 14:13 ` Peter Stephenson 2018-06-19 14:38 ` Vincent Lefevre 2018-06-19 15:59 ` Peter Stephenson
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).