[prev in list] [next in list] [prev in thread] [next in thread] 

List:       opensolaris-tools-discuss
Subject:    Re: [tools-discuss] [review] 6570962 nightly -O should include
From:       Mike Kupfer <mike.kupfer () sun ! com>
Date:       2007-06-30 21:26:21
Message-ID: 20903.1183238781 () athyra
[Download RAW message or body]

Hi Roland, thanks for the comments.

>>>>> "Roland" == Roland Mainz <roland.mainz@nrubsig.org> 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'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 and (b) simply to conform to a not-yet-blessed style guide.

>> @@ -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.

>> +cpio_filt() {
>> +       typeset cpio_stat

Roland> Please use "integer cpio_stat" since the value is always an
Roland> integer ("makebfu_filt()" needs the same fiy).

Okay.

>> +       ?)
>> +               echo "usage: $usage"
>> +               exit 1
>> +               ;;

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.

>> +[ $? -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).

>> +mkdir -p $stagedir/$subdir/$MACH || \

Roland> Please put the $stagedir/$subdir/$MACH in quotes,

Will fix.

>> 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.  I'm going to leave this code
as-is.

>> + grep -v "^Creating .* archive:" $tmplog | grep -v "^Making" | \
>> + grep -v "^$" | sort | uniq > $errors

Roland> Use "sort -u" instead of "sort | uniq"

Okay.

mike
_______________________________________________
tools-discuss mailing list
tools-discuss@opensolaris.org
[prev in list] [next in list] [prev in thread] [next in thread]