diff mbox

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

Message ID 1556112453-20386-1-git-send-email-yejun.guo@intel.com
State Superseded
Headers show

Commit Message

Guo, Yejun April 24, 2019, 1:27 p.m. UTC
take decoder names an example, with the default page length, shell command
'pr' needs two pages for all the decoder names. The names are firstly printed
in the first page, then in the second page. So, as a whole, the names are
sorted neither in column order nor in row order. It's a little confused.

One method is to calculate the proper page length, so all the names are printed
in one page by 'pr -l', and so strictly in alphabet order, column by column.

Another method is to use command printf instead of pr, because buybox doesn't
have pr. This patch refines print_in_columns to print the names with printf
in alphabet order, very similar with 'pr -l', except the case when the last
column is not fully filled with names.

contributor: Alexander Strasser <eclipse7@gmx.net>
contributor: avih <avihpit-at-yahoo.com@ffmpeg.org>
Signed-off-by: Guo, Yejun <yejun.guo@intel.com>
---
 configure | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

avih April 24, 2019, 6:55 a.m. UTC | #1
> 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.

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

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

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

- `$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.

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


> 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.
diff mbox

Patch

diff --git a/configure b/configure
index 3b11ffe..8941063 100755
--- a/configure
+++ b/configure
@@ -3832,8 +3832,25 @@  die_unknown(){
 }
 
 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/ *$//'
 }
 
 show_list() {