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

Alexander Strasser eclipse7 at gmx.net
Sun Apr 28 04:18:18 EEST 2019


Hi both of you,

nice work iterating on this patch set!

I want to discuss a little more, please don't take my comments
the wrong way, I just couldn't get back to this discussion earlier.

On 2019-04-24 13:36 +0000, Guo, Yejun wrote:
> >
> > From: avih [mailto:avihpit at yahoo.com]
> > Sent: Wednesday, April 24, 2019 9:22 PM
> > To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> > Cc: Guo, Yejun <yejun.guo at intel.com>
> > Subject: Re: [FFmpeg-devel] [PATCH V5 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
> > > +    # the input should not contain chars such as '*',
> > > +    # otherwise, '*' will be expanded to be all files in the current
> > > +    # working directory which don't begin with a dot (`.`)
> > > +    set -- $(tr ' ' '\n' | sort)
> > > +    col_width=24
> > > +    if [ $ncols -lt $col_width ]; then
> > > +        col_width=$ncols
> > > +    fi
> > > +    cols=$(($ncols / $col_width))
> > > +    rows=$(($(($# + $cols - 1)) / $cols))
> > > +    cols_seq=$(seq $cols)
> > > +    rows_seq=$(seq $rows)
> > > +    for row in $rows_seq; do
> > > +        print_index=$row
> > > +        print_line=""
> > > +        for col in $cols_seq; do
> > > +            if [ $print_index -le $# ]; then
> > > +                eval print_line='"$print_line "${'$print_index'}'
> > > +            fi
> > > +            print_index=$(($print_index + $rows))
> > > +        done
> > > +        printf "%-${col_width}s" $print_line
> > > +        printf "\n"
> > > +    done | sed 's/ *$//'
> > > }
> >
> > Looks good to me. No further comments (but I don't push).
> >
> > Next time, know that you can use e.g. `$((x + y))` instead of `$(($x + $y))`,
> > though in this case it doesn't matter and not worth another version.
>
> got it, thanks.

What do you think about using awk instead of shell?

I have 2 POC patches attached. It's probably not 100% correct yet,
but it kind of demonstrates what it would look like.

The main advantage of using awk, is that it's more elegant and
shorter. It's probably also less risky, because it's more isolated,
e.g. as it was explained by avih, there is no widely supported way
for locals across shells. We already use awk in configure for
mandatory functions, so it's no new dependency.

Please comment on the awk approach.

I'm not against the shell way, or a mixed approach, but before going
either way and pushing I would rather have some more testing;
especially on more exotic platforms.


Thank you
  Alexander
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-configure-print_in_columns-Replace-pr-with-awk.patch
Type: text/x-diff
Size: 1502 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190428/9d8a1a45/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-configure-log_file-Replace-pr-with-awk-invocation.patch
Type: text/x-diff
Size: 614 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190428/9d8a1a45/attachment-0001.patch>


More information about the ffmpeg-devel mailing list