Re: [PATCH 3/3] ash: use JOBS as the compile time option it actually is

[Johannes Schindelin]

Hi Denys,

On Fri, 14 Jul 2017, Denys Vlasenko wrote:

> On Fri, Jul 14, 2017 at 4:11 PM, Johannes Schindelin
>  wrote:
> > Using the constant in a regular `if (...)` pretends that this is
> > anything but a compile time option. Let's use `#if JOBS` instead,
> > making it *much* clearer what `JOBS` actually is.
> >
> > Incidentally, this change fixes the build in setups where there are
> > no headers defining WIFSTOPPED and WSTOPSIG (where JOBS has to be
> > set to 0).
> >
> > This partially reverts 4700fb5be (ash: make dowait() a bit more
> > readable. Logic is unchanged, 2015-10-09).
> 
> Some people (not me) have the completely opposite POV
> and used to ask me to remove excessive #ifs.
> 
> It's difficult to find a middle ground here...

Sure.

But when something cannot build because of missing symbols, and there is
not even a compile time flag to fix it (even when the compile time flag
clearly switches on/off users of the symbols in question), I feel that
there is no need for middle ground.

If it fixes the build, it fixes the build.

The patches in this patch series fix the build here.

Ciao,
Johannes
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 3/3] ash: use JOBS as the compile time option it actually is

[Johannes Schindelin]

Hi Xabier,

On Fri, 14 Jul 2017, Xabier Oneca  --  xOneca wrote:

> 2017-07-14 16:11 GMT+02:00 Johannes Schindelin :
> > @@ -4182,7 +4186,9 @@ dowait(int block, struct job *job)
> > goto out;
> > }
> > /* The process wasn't found in job list */
> > -   if (JOBS && !WIFSTOPPED(status))
> > +#if JOBS
> > +   if (!WIFSTOPPED(status))
> > +#endif
> > jobless--;
> 
> Here the logic does change.

Whoops, you're right, the #endif should enclose the jobless--; statement.

Will fix,
Johannes
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 3/3] ash: use JOBS as the compile time option it actually is

[Denys Vlasenko]

On Fri, Jul 14, 2017 at 4:11 PM, Johannes Schindelin
 wrote:
> Using the constant in a regular `if (...)` pretends that this is
> anything but a compile time option. Let's use `#if JOBS` instead,
> making it *much* clearer what `JOBS` actually is.
>
> Incidentally, this change fixes the build in setups where there are
> no headers defining WIFSTOPPED and WSTOPSIG (where JOBS has to be
> set to 0).
>
> This partially reverts 4700fb5be (ash: make dowait() a bit more
> readable. Logic is unchanged, 2015-10-09).

Some people (not me) have the completely opposite POV
and used to ask me to remove excessive #ifs.

It's difficult to find a middle ground here...
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 3/3] ash: use JOBS as the compile time option it actually is

[Xabier Oneca -- xOneca]

Hello Johannes,

2017-07-14 16:11 GMT+02:00 Johannes Schindelin :
> Using the constant in a regular `if (...)` pretends that this is
> anything but a compile time option. Let's use `#if JOBS` instead,
> making it *much* clearer what `JOBS` actually is.
>
> Incidentally, this change fixes the build in setups where there are
> no headers defining WIFSTOPPED and WSTOPSIG (where JOBS has to be
> set to 0).
>
> This partially reverts 4700fb5be (ash: make dowait() a bit more
> readable. Logic is unchanged, 2015-10-09).
>
> Signed-off-by: Johannes Schindelin 
> ---
>  shell/ash.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/shell/ash.c b/shell/ash.c
> index 8c2098dd9..5c8216331 100644
> --- a/shell/ash.c
> +++ b/shell/ash.c
> @@ -4022,15 +4022,19 @@ sprint_status48(char *s, int status, int sigonly)
>
> col = 0;
> if (!WIFEXITED(status)) {
> -   if (JOBS && WIFSTOPPED(status))
> +#if JOBS
> +   if (WIFSTOPPED(status))
> st = WSTOPSIG(status);
> else
> +#endif
> st = WTERMSIG(status);
> if (sigonly) {
> if (st == SIGINT || st == SIGPIPE)
> goto out;
> -   if (JOBS && WIFSTOPPED(status))
> +#if JOBS
> +   if (WIFSTOPPED(status))
> goto out;
> +#endif
> }
> st &= 0x7f;
>  //TODO: use bbox's get_signame? strsignal adds ~600 bytes to text+rodata
> @@ -4182,7 +4186,9 @@ dowait(int block, struct job *job)
> goto out;
> }
> /* The process wasn't found in job list */
> -   if (JOBS && !WIFSTOPPED(status))
> +#if JOBS
> +   if (!WIFSTOPPED(status))
> +#endif
> jobless--;

Here the logic does change.

>   out:
> INT_ON;
> --
> 2.13.3.windows.1.13.gaf0c2223da0
> ___
> busybox mailing list
> busybox@busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox

Cheers,

Xabier Oneca_,,_
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox