Date: Sun, 2 Dec 2001 01:52:04 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Wes Peters <wes@softweyr.com> Cc: Bill Fenner <fenner@research.att.com>, <mike@FreeBSD.org>, <freebsd-standards@bostonradio.org> Subject: Re: strerror_r() implementation Message-ID: <20011202011045.F5026-100000@gamplex.bde.org> In-Reply-To: <3C0881FE.9CA0DD90@softweyr.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 1 Dec 2001, Wes Peters wrote: > Bruce Evans wrote: > > > > I prefer the version enclosed at the end. It has the following additional > > non-formatting changes: > > - parametrize the buffer size according to the sizes in strerror_r() > > ...but do it in a way that doesn't break the function. I've tested it a bit and have only found one cosmetic bug, an old one: the prefix for strerror(0) is "Undefined error: " (from sys_errlist[0]), but the prefix for other unusual errors is "Unknown error: " (from the function). One more cosmetic bug turned up in this thread, a not so old one: adding the length of the prefix to the size of the temporary buffer is bogus. > > - unobfuscate uerr (don't use an unsigned variable just to hand-optimize > > for 20-year-old compilers). > > When did "don't change code that is known to work" get thrown out? > uerr is used because the simplistic ASCII conversion doesn't really > grok signedness. It does do quite a nice job in very little code. When it's badly written. uerr is used in strerror() because it was copied from strerror_r(). strerror() doesn't do any integer to string conversions, so using uerr in it is just an obfuscation. > > - remove redundant code (the strerror_r() can't fail unless we used the > > wrong buffer size, and we should get this right by not hard-coding the > > size of tmp[]) > > `tmp' should have a better name now that it is more global. I didn't > > change it because that would require large changes in strerror_r(). > > It's a throw-away character buffer and making it more global is an error. Either it or its size needs to be more global so that knowledge of its size doesn't need to be hard-coded into strerror(). > > > The last change is a whitespace error. Mustn't have any style(9) nits, > > > right? ;^) > > > > I agree, so about 10 more of them in strerror() alone ;^). They were in > > the following classes: > > (1) missing parens around return values > > style(9) remains silent on this subject. Until required to do so by It is not silent, but not very verbose. All of the examples of returning a value in style(9) use "return (foo)". There is a whole one such example. It apparently didn't occur to the origianal author of style(9) that this needed an explicit rule. > style(9) I will not submit to this barbarism. A quick check of other > routines in libc/string showed about 50% usage of return (foo) vs. > return foo. libc/string might not be a good example. The best examples are: sys/kern (CSRG version), libc/stdio (CSRG version), and contrib/vi for a slightly more modern version, or even the CSRG version of libc/string. > > (2) comments not English sentences > > Mine were. Existing ones often were not. Except for: /* strerror can't fail so handle truncation semi-elegantly */ > > (3) excessive vertical whitespace (KNF uses indent -sob) > > This would be much more understandable if style(9) spelled out what is > "acceptable" or "required" vertical whitespace. As it currently stands, > there is no mention of the word "vertical" anywhere, and no mention of > "blank line" after the description of include files. Again, existing > usage in the libc/string directory is all over the map, and style(9) > is completely opaque on the subject. You have to look at some KNF code. The old version of strerror.c is a good enough example here. It uses doesn't use any blank lines except before the block of code begun by a comment. This was intentional. > > (4) excessive indentation for comments at the right of code (32 is normal). > > The only mention of right-comments in style(9) simply says "Try to align > the comments". > > style(9) would be much easier to adhere to if it were actually documented > in style(9), rather than being the unagreed-upon figment of the imagination > of several dozen different people. It might be simpler to fix indent(1) and filter commits through it. > > %%% > > Index: strerror.c > > ... > > This has not been tested. > > Please clarify this comment? "I rebuilt libc to check that this compiles, but have not installed libc or done any runtime tests". I tested it a bit more and found a bug in the test code: the test of a short buffer returns an unterminated string (as expected), and printing this non-string using %s gives garbage. Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-standards" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20011202011045.F5026-100000>