Date: Sun, 19 Apr 2015 18:23:26 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Eitan Adler <eadler@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r281724 - head/usr.bin/rpcgen Message-ID: <20150419172929.F1928@besplex.bde.org> In-Reply-To: <201504190453.t3J4rTQT057322@svn.freebsd.org> References: <201504190453.t3J4rTQT057322@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150419172929.F1928>