svn commit: r281758 - head/bin/ed

[Bruce Evans]

On Mon, 20 Apr 2015, Eitan Adler wrote:

> On 19 April 2015 at 21:23, Bruce Evans <brde at optusnet.com.au> wrote:
>> On Mon, 20 Apr 2015, Eitan Adler wrote:
>>> ...
>>> --- head/bin/ed/glbl.c  Mon Apr 20 00:24:32 2015        (r281757)
>>> +++ head/bin/ed/glbl.c  Mon Apr 20 02:07:57 2015        (r281758)
>>> @@ -60,7 +60,7 @@ build_active_list(int isgcmd)
>>>                         return ERR;
>>>                 if (isbinary)
>>>                         NUL_TO_NEWLINE(s, lp->len);
>>> -               if (!regexec(pat, s, 0, NULL, 0) == isgcmd &&
>>> +               if (!(regexec(pat, s, 0, NULL, 0) == isgcmd) &&
>>>                     set_active_node(lp) < 0)
>>>                         return ERR;
>>>         }
>>
>>
>> How can this be right?  !(a == b) is an obfuscated way of writing a != b.
> bah!

> How does something like the following look?

Sorry, not good.

I don't like stdbool.  Here using it just enlarges and unportablize the
code.

> Index: ed.h
> ===================================================================
> --- ed.h    (revision 281759)
> +++ ed.h    (working copy)
> @@ -33,6 +33,7 @@
> #include <limits.h>
> #include <regex.h>
> #include <signal.h>
> +#include <stdbool.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>

ed already has bad style, with *.h included in ed.h...

> @@ -191,7 +192,7 @@ int put_des_char(int, FILE *);
> void add_line_node(line_t *);
> int append_lines(long);
> int apply_subst_template(const char *, regmatch_t *, int, int);
> -int build_active_list(int);
> +int build_active_list(bool);

It would be impossible to even declare a function that uses bool
in a header without increasing the pollution.

Actually, this is easy in C99 and only difficult to do portably.
_Bool is standard in C99, so the include is only needed for portability
to systems that have <stdbool.h> but not _Bool, or if you want to spell
_Bool as bool in C99.

The header (and program) otherwise doesn't use many fancy types.

> int cbc_decode(unsigned char *, FILE *);
> int cbc_encode(unsigned char *, int, FILE *);
> int check_addr_range(long, long);

It does use FILE.  That requires the <stdio.h> pollution.

> Index: glbl.c
> ===================================================================
> --- glbl.c    (revision 281759)
> +++ glbl.c    (working copy)
> @@ -60,7 +60,7 @@ int
>             return ERR;
>         if (isbinary)
>             NUL_TO_NEWLINE(s, lp->len);
> -        if (!(regexec(pat, s, 0, NULL, 0) == isgcmd) &&
> +        if ((!regexec(pat, s, 0, NULL, 0)) == isgcmd &&
>             set_active_node(lp) < 0)
>             return ERR;
>     }

This still has the obfuscation of using ! on a non-boolean, and then
needs excessive parentheses to defeat the compiler warning about
(a side effect of) this.

Parentheses are needed for syntactical reasons in the non-obfuscated
version:

 	if ((regexec(pat, s, 0, NULL, 0) == 0) == isgcmd &&

This depends on the value of isgcmd being 0 or 1.  It obviously is,
but we have to examine all the callers (just 1) to see this.  The
bool declaration lets the compiler ensure this.  You can ensure it
directly replacing isgcmd by (isgcmd != 0).

Maybe I wouldn't object to using bool if you rewrote ed to use it in
all places that use booleans.  ed doesn't seem to have many such places.
It uses the variable "int gflag" a lot.  gflag contains 5 flags.  These
are essentially packed booleans.  It would be silly to use booleans for
the easy case of 1 truth value while continuing to use ints for more
complicated cases, but I think that if you translate the complicated
cases to use booleans then they would be even more complicated.

Bruce