[FFmpeg-devel] [PATCH V4 1/2] configure: sort decoder/encoder/filter/... names in alphabet order

[Guo, Yejun]

> -----Original Message-----
> From: Guo, Yejun
> Sent: Wednesday, April 24, 2019 3:29 PM
> To: Guo, Yejun <yejun.guo at intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH V4 1/2] configure: sort
> decoder/encoder/filter/... names in alphabet order
> 
> > print_in_columns() {
> > -    cols=$(expr $ncols / 24)
> > -    cat | tr ' ' '\n' | sort | pr -r "-$cols" -w $ncols -t
> > +    set -- $(cat | tr ' ' '\n' | sort)
> > +    col_width=24
> > +    cols=$(($ncols / $col_width))
> > +    rows=$(($(($# + $cols - 1)) / $cols))
> > +    cols_seq=$(seq $cols)
> > +    rows_seq=$(seq $rows)
> > +    for row in $rows_seq; do
> > +        index=$row
> > +        line=""
> > +        fmt=""
> > +        for col in $cols_seq; do
> > +            if [ $index -le $# ]; then
> > +                eval line='"$line "${'$index'}'
> > +                fmt="$fmt%-${col_width}s"
> > +            fi
> > +            index=$(($index + $rows))
> > +        done
> > +        printf "$fmt\n" $line
> > +    done | sed 's/ *$//'
> > }
> 
> Looks good.
> 
> Hopefully last batch of comments, nothing critical:
> 
> - Make sure the new code behaves well when $ncols < 24. It's an unlikely case
>   but I think v3 and v4 don't print anything if that happens.

yes, there will be script error with division by zero. I'll add 'if' here.

> 
> - You don't need to "build" `$fmt`, because printf reuses the format string
>   if there are more values than placeholders. Just use one fixed "%-24s".

thanks, will change to it.

> 
> - The `cat |` in `cat | tr ' ' '\n' | sort` is not required. It just adds
>   a process without giving any value.

thanks, will remove 'cat |'.

> 
> - When `$(<some-commands>)` is unquoted and not part of an assignment then
> it's
>   also subject to glob expansion. E.g. try `printf "A B *" | print_in_columns`
>   with your code compared to the original version with `pr`. For our case it's
>   not an issue because the values do not expand (and other parts in configure
>   will break if they did expand), but in general the correct (and slower) way
>   to handle it is with `read`, so it's worth adding a comment that there are
>   some constraints with the values.

got it, will add the comments.

btw, I tried read as following, looks that read also expands '*':
print_in_columns() {
	read inputs
	echo $inputs
}

echo "xxxxxxxxxxxxx"
echo "A B *" | print_in_columns
echo "yyyyyyyyyyyyy"

> 
> - `$rows_seq` is not required (but also doesn't hurt). It's evaluated only once
>   anyway - only when the outer loop starts, unlike `$cols_seq` which is used
>   with a new `for` on each row. But it doesn't have a performance impact and
>   it's a good practice in general where possible, so it's fine to keep it.

ok, will keep it.

> 
> - To measure the runtime of a program you can do `time program
> <arguments>`.
>   To measure the runtime of sections in a script you can use `time.sh` which I
>   used with `configure` in the past - https://github.com/avih/time.sh .

thanks for your nice tool!

> 
> 
> > To check a name with human eye, after we get the nearby area for the check:
> > If row by row, we have to turn our eyes from left to right, from up to down,
> > and then again left to right, up to down, maybe again and again. And there is
> still
> > possibility that we missed the check.
> >
> > If column by column, we just turn from up to down (the same column) and
> > can easily check if the name is there or not.
> 
> Yeah, no disagreement there. Just wondered if it was considered. Thanks.