> -----Original Message-----
> From: Guo, Yejun
> Sent: Wednesday, April 24, 2019 3:29 PM
> To: Guo, Yejun <yejun....@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
