Roland Mainz wrote:
> Vladimir Kotal wrote:
>> Vladimir Kotal wrote:
>>
>> <snip>
>>
>>> ok, I will rewrite the tests in nc test suite to use double brackets.
>> After reading the shell style document I made the following changes:
>> - used double square brackets for if tests
>> - converted escape characters to $( )
>> - converted function definitions to 'function { }'
>> - fixed if/for style
>> - then/do on the same line as the check
>> - semicolon separated by space from ']]'
>>
>> new webrev is here:
>>
>> http://cr.opensolaris.org/~vkotal/nc-tet.onnv-stc2-restructured-ksh_style/
>
> Quick 5min race over
> http://cr.opensolaris.org/~vkotal/nc-tet.onnv-stc2-restructured-ksh_style/nc-tet.onnv-stc2.patch
> (patch code is quoted with '> ')
>
>> +++ new/src/suites/net/nc/src/lib/ksh/common_funcs.ksh
>
> General: What about using FPATH (function path) to load these common
> functions on demand ?
Any example of how to do that properly ?
>> + if [[ $? -ne 0 ]] ; then
>
> Please use
> -- snip --
> if (( $? != 0 )) ; then
> -- snip --
Again, this still has the space between semicolon and '))'. It seems the
majority uses similar constructions without the space.
> (since $? is an integer variable - see
> http://www.opensolaris.org/os/project/shell/shellstyle/#compare_exit_code_using_math)
I have fixed all occurrences of those, including -eq, -lt, -gt, -le and -ge.
>> +function common_cleanup
>> +{
>> + debug "Cleanup"
>> + stop_nc $@
>> +}
>
> AFAIK you want:
> -- snip --
> stop_nc "$@"
> -- snip --
> (http://www.opensolaris.org/os/project/shell/shellstyle/#store_lists_in_arrays
> has a note about the difference between "$*" vs. "$@") ...
Fixed.
>> +function finish_test
>> +{
>> + common_cleanup $@
>> +}
>
> See above...
fixed.
> +function debug
> +{
> + [ $STC2_NC_DEBUG -gt 0 ] && tet_infoline "$*"
> +}
>
> AFAIK you want
> -- snip --
> (( STC2_NC_DEBUG > 0 )) && tet_infoline "$*"
> -- snip --
fixed.
> ... I'm not sure about the "$*" - does the script wants to pass all
> arguments to the "debug" function as _single_ string to "tet_infoline" ?
yes.
>
> BTW: It may be usefull to replace the values "0" and "1" with "true" and
> "false" - it would allow you to do this:
> -- snip --
> $STC2_NC_DEBUG && tet_infoline "$*"
> -- snip --
ok, leaving this as is for now.
>> +function debug_printfile
>> +{
>> + if [[ $STC2_NC_DEBUG -eq 1 ]] ; then
>
> Please use (( STC2_NC_DEBUG == 1 )) ...
this got fixed together with all the arithmetics.
>> + if [[ -r "$1" ]] ; then
>> + tet_printfile "$1"
>> + else
>> + tet_infoline "cannot print file '$1' (not readable)"
>> + fi
>> + fi
>> +}
>
>> +function print_test_case
>> +{
>> + unset ptc_short_info
>> + typeset tc_id="$1"
>> + typeset tc_descr="$2"
>> + typeset ptc_info="Test case $tc_id - $tc_descr"
>> + typeset -ir columns=60
>
> Please use "integer -r columns=60".
all occurences of 'typeset -i','typeset -ri' were substituted with
'integer' or 'integer -r', respectively.
>> +
>> + tet_infoline
>> "======================================================="
>> +
>> + if [[ $( echo $ptc_info | $WC -c ) -gt $columns ]] ; then
>
> 1. "echo" will interpret backslash characters in "$ptc_info" ... is that
> wanted ?
most probably not.
> 2. "echo" is not portable
> 3. "wc -c" counts in _bytes_, not _characters_ (see
> http://www.opensolaris.org/os/project/shell/shellstyle/#shell_uses_charatcers_not_bytes)
> 4. Please don't use string operators to compare numbers
>
> AFAIK it may be better to write (ksh88+ksh93):
> -- snip --
> if (( $( printf "%s\n" "$ptc_info" | $WC -C ) > columns )) ; then
> -- snip --
fixed. This still does not solve the 'bytes vs. characters' potential
problem. Any idea how to approach this in correct way ?
> or (ksh93-only)
> if (( $( $WC -C <<<"$ptc_info" ) > columns )) ; then
>
>> + #
>> + # Split the line
>> + #
>> + typeset -i ptc_ltrcnt=0
>
> Please use "integer" instead of "typeset -i" ("integer" refers to the
> largest integer datatype supported by the shell interpreter, e.g. 32bit
> for ksh88 until Solaris 10 and then 64bit for ksh88 on Solaris 10 and
> 64bit for ksh93 (may grow in the future)).
fixed with all integer substitutions.
>> + for ptc_word in $ptc_info; do
>> + ptc_wordsz=$( echo $ptc_word | $WC -c )
>
> See above...
fixed.
>> + ptc_ltrcnt=$(($ptc_ltrcnt + $ptc_wordsz + 1))
>
> Plese use:
> -- snip --
> (( ptc_ltrcnt=ptc_ltrcnt + ptc_wordsz + 1))
> -- snip --
this construction looks a bit weird to me because IMHO it makes the
script less readable. It defines the variable and assigns the value into
it. If it was something like (( i = i + 1 )) then it would be ok but in
this case I cannot say I like it.
What's wrong with the original version ?
>> + if [[ $ptc_ltrcnt -gt $columns ]] ; then
>
> Please use:
> -- snip --
> if (( ptc_ltrcnt > columns )) ; then
> -- snip --
fixed.
>> + tet_infoline "$ptc_short_info"
>> + ptc_short_info=" $ptc_word"
>> + ptc_ltrcnt="$ptc_wordsz"
>> + else
>> + ptc_short_info="$ptc_short_info $ptc_word"
>> + fi
>> + done
>> + if [[ $ptc_ltrcnt -gt 0 ]] ; then
>
> Please use:
> -- snip --
> if (( ptc_ltrcnt > 0 )) ; then
> -- snip --
fixed.
>> + tet_infoline "$ptc_short_info"
>> + fi
>> + else
>> + tet_infoline "$ptc_info"
>> + fi
>> +
>> + tet_infoline
>> "======================================================="
>> +}
>
> AFAIK the rest of the patch just repeats the issues above over and over
> again...
okay. Thanks. These were very valuable comments.
webrev has been refreshed:
http://tessier.czech.sun.com/nc-tet.onnv-stc2/
here's incremental webrev against previous revision:
http://cr.opensolaris.org/~vkotal/nc-tet.onnv-stc2.rolands_comments/
v.
