Mike Kupfer wrote: > > Hi Roland, thanks for the comments. > > >>>>> "Roland" == Roland Mainz <[EMAIL PROTECTED]> writes: > > Roland> Only some random nitpicking (following > Roland> http://www.opensolaris.org/os/project/shell/shellstyle/): > > What's the status of this document? It currently says that it's a > draft, and "subject to change without notice".
I wrote that because I didn't merge all of David Korn's suggestions yet (see http://mail.opensolaris.org/pipermail/shell-discuss/2007-June/000993.html), a few items are still missing (like that all scripts+functions should make sure they return a correct exit code, zero for success and a defined non-zero value otherwise (most scripts don't care and return the value of the last command executed... which can be quite annoying if you use such a script within other scripts and try to rely on the exit code of the embedded script) and I am "at war" with the DocBook markup right now... somehow it doesn't look good yet and the default opensolaris.org CSS doesn't make the resultng HTML look much better... ;-/ > I'm happy to fix things > that are bugs (like the missing double quotes), but I'm reluctant to > make purely stylistic changes that are (a) inconsistent with the rest of > the script Well, IMO the rest of the script looks like a giant piece of mixture of 7/8 bourne and 1/8 korn shell code... ;-( > and (b) simply to conform to a not-yet-blessed style guide. Well, I've send Mike Shapiro an email a while ago to comment on the new styleguide but didn't receive any answers yet... ;-( > >> @@ -44,10 +49,14 @@ > >> > >> [ -n "$SRC" ] || fail "Please set SRC." > >> [ -n "$CODEMGR_WS" ] || fail "Please set CODEMGR_WS." > >> +[ -n "$CPIODIR" ] || fail "Please set CPIODIR." > > Roland> IMO it may be better to use [[ ]] since bfudrop.sh is a ksh > Roland> script. > > And the current style guide already mentions [[ ]] as being preferred. > (I'd missed that the last time I reviewed the style guide.) I'll fix > this and the other instances. BTW: [[ ]] allows the use of && and || instead of -a and -o (more or less stylistic but may be easier for people who work more with C code than shell scripts) ... [snip] > Roland> s/echo/print/ for ksh scripts (this is repeated many times in > Roland> the patch) > > This looks like a stylistic change that is contrary to the existing > code. Is there some other benefit to using print? Note that the > echo(1) man page says that "echo" is a built-in for ksh. Erm, that's not true for all variants of the korn shell. The problem with "echo" is that several variants work differently (basically BSD "echo" vs. SysV "echo" with per-Unix variations and/or add-ons or hacks for backwards-compatibilty to stuff which is dead and will stay dead even if all zombies raise from their graves), making it almost impossible to write a portable scripts without running into this problem (even on Solaris where you have both /usr/bin/echo, /usr/ucb/echo and various builtin command versions in the shells). David Korn solved the issue via adding a new bulitin called "print" which merges functionality of both BSD and SysV "echo" and partially "printf" (for ksh93, however ksh93 has a pure "printf" builtin, too), the builtin "echo" command should behave like the OS's native "echo" command (in theory... unfortunately there are subtle differences...). This issue will even get more nasty in the future - the current ksh93 "echo" builtin is not bound to a path, however to conform to the POSIX shell standard it needs to be bound to one (Don Cragun pointed this out in a phone call long ago when we were planning the first ksh93 ARC case) which means the future availabilty of the "echo" builtin in the korn shell will depend on he content of "${PATH}" (oh, and another twist: The ksh93 "echo" supports _both_ flavors - if you bind it to /usr/bin/echo it will behave like Solaris "echo", if you bind it to /usr/ucb/echo it will behave like the BSD one). Or short: There are basically three options: 1. Use "echo" and pray that /usr/bin/ is in your ${PATH} 2. Use the full path the "echo" command to make sure you're picking the right one, e.g. "/usr/bin/echo" 3. Use the "print" builtin (which is guranteed to be a builtin and explictly not bound to a path (and therefore skips the whole path lookup for builtins which are bound to a specific path)) (BTW: See David Korn's comment in http://mail.opensolaris.org/pipermail/shell-discuss/2007-June/000993.html , item 4) > >> +[ $? -eq 0 ] || fail "can't copy original proto area." > > Roland> ksh script, please use (( )) for arithmetric expressions, > Roland> e.g. (( $? == 0 )) || fail ... > > I would like to wait until this aspect of the style guide is finalized > (and formally adopted by ON). OK (technically the idea was to make the code faster... [ ] and [[ ]] operate on strings while (( )) operate on numbers (see below)) ... [snip] > >> shift $(($OPTIND - 1)) > > Roland> shift $((OPTIND - 1)) # should work, too (e.g. within (( > Roland> ))-expressions the '$' is not needed for variables)... > > This was not clear to me from the ksh(1) man page, and it seems likely > to confuse future maintainers of the code. Why should it be confusing ? $(( )) is for arithmetric expressions and in such cases the variable name do not need the '$'-prefix. Basically the code $(($OPTIND - 1)) fetches the integer variable "OPTIND", turns it into a string, then feeds the string to the $(( )) expression which converts it to an integer value. The code $((OPTIND - 1)) just reads the integer value of the variable "OPTIND" directly without the whole conversion bloodbath first... BTW: Watch the difference when running the script with "set -o xtrace" ... > I'm going to leave this code as-is. Ok... ;-( ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) [EMAIL PROTECTED] \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;) _______________________________________________ tools-discuss mailing list tools-discuss@opensolaris.org
