[PATCH 2/2] Fix two C nits

[PATCH 2/2] Fix two C nits

[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

Re: [PATCH 2/2] Fix two C nits

[Peter Stephenson]

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

Re: [PATCH 2/2] Fix two C nits

[Eitan Adler]

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

Re: [PATCH 2/2] Fix two C nits

[Vincent Lefevre]

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)

Re: [PATCH 2/2] Fix two C nits

[Peter Stephenson]

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

Re: [PATCH 2/2] Fix two C nits

[Vincent Lefevre]

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)

Re: [PATCH 2/2] Fix two C nits

[Peter Stephenson]

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