svn commit: r281724 - head/usr.bin/rpcgen

[Bruce Evans]

On Sun, 19 Apr 2015, Eitan Adler wrote:

> Log:
>  rpcgen: fix use use of strcmp
>  	strcmp only guarantee that it will return at least 1 if the string B
>  	is greater than that of string A.
>
> Modified:
>  head/usr.bin/rpcgen/rpc_sample.c
>
> Modified: head/usr.bin/rpcgen/rpc_sample.c
> ==============================================================================
> --- head/usr.bin/rpcgen/rpc_sample.c	Sun Apr 19 04:27:50 2015	(r281723)
> +++ head/usr.bin/rpcgen/rpc_sample.c	Sun Apr 19 04:53:28 2015	(r281724)
> @@ -115,7 +115,7 @@ write_sample_client(const char *program_
> 			for (l = proc->args.decls; l != NULL; l = l->next) {
> 				f_print(fout, "\t");
> 				ptype(l->decl.prefix, l->decl.type, 1);
> -				if (strcmp(l->decl.type,"string") == 1)
> +				if (strcmp(l->decl.type,"string") >= 1)
> 				    f_print(fout, " ");
> 				pvname(proc->proc_name, vp->vers_num);
> 				f_print(fout, "_%s;\n", l->decl.name);

It is still a bad example.  strcmp is actually documented as returning > 0
if <the condition above>.  ' >= 1' is just an obfuscated way of writing
' > 0'.

Actually, it is still nonsense.  I think l_decl.type is a keyword with
a limited number of values.  Probably 1 was never returned, since there
is no keyword starting with "r" or "sr".  So to avoid changing the
behaviour, the correct change was to remove the dead code.

However, the behaviour was probably broken.  Signed comparison with
keywords makes no sense.  Only comparison for equality, or perhaps
for being a prefix, makes sense.  I think comparison for equality was
intended.  That could be written as 'strcmp ... == 0'.

However, using strcmp to compare for equality would be a style bug.
Everywhere else in the file, and almost everywhere else in the program
spell this comparison using streq, since strcmp is apparently too hard
to use.  The other broken places are:

- rpc_cout.c:   3 instances of strcmp,  9 of streq
- rpc_main.c:   4 instances of strcmp,  4 of streq
- rpc_parse.c:  2 instances of strcmp, 11 of streq
- rpc_sample.c: 1 instances of strcmp,  6 of streq
- rpc_svcout.c: 3 instances of strcmp,  5 of streq
- rpc_util.c:   2 instances of strcmp,  9 of streq

streq is used fairly consistently in old code, and the style was broken
fairly consistently in changes (about half, including the above,
apparently from Sun in 1995).

There are often mounds of collateral style bugs near changes to use
strcmp.  In the above, the next statement is misindented.  In this file,
the indentation elsewhere is mostly using tabs.  It is never using
spaces where that would be correct, but is often where it is incorrect
(mostly for corrupted tabs and 2-space indentation).  Old sun code had
many more indentation errors from 5-space indentation for K&R function
headers.  Altogether, this file had a mixture of 2-space indentation,
4-space indentation which gave bogus lining up of parentheses (in
the above code), 5-space indentation, correct lining-up-parentheses
indentation, 8-space indentation from corrupt tabs, and normal indentation
with tabs.  A grate sample.

Bruce