Re: [PATCH v2] stdc: some improvements

[Alejandro Colomar <alx.manpages@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8197 bytes --]

Hi Oskari, Sam,

On 3/24/23 06:07, Oskari Pirhonen wrote:
> - Some small stylistic changes such as removing trailing semicolons and
>   noisy `shift` calls.

But they are sooo pretty =)  That's a little OCD of mine putting
semicolons in shell scripts.  Maybe I read too much C, but it feels
good to see punctuation marks at the end of sentences.  I prefer to
keep semicolons.

> - Ensure pcregrep exists. It is installed as pcre2grep on my machine, so
>   check for both and error out if neither is found. Prefer pcre2grep
>   (installed by libpcre2) because libpcre is EOL.

Hmm, I didn't know about pcre2grep(1).  I've applied the following:

commit 40049b4d55f6ba2f392aa3b48835b4555ef79ee2 (HEAD -> main, alx/main)
Author: Alejandro Colomar <alx@kernel.org>
Date:   Fri Mar 24 12:39:33 2023 +0100

    bin/: Use pcre2grep(1) instead of pcregrep(1)
    
    PCRE is EOL, and replaced by PCRE2.
    
    Reported-by: Oskari Pirhonen <xxc3ncoredxx@gmail.com>
    Reported-by: Sam James <sam@gentoo.org>
    Signed-off-by: Alejandro Colomar <alx@kernel.org>

diff --git a/bin/stdc b/bin/stdc
index 8c725a2..e70e067 100755
--- a/bin/stdc
+++ b/bin/stdc
@@ -14,7 +14,7 @@ err()
 
 grep_proto()
 {
-       pcregrep -M "(?s)\b$1 *\([[:alnum:]*,._\s\(\)-]*\);$";
+       pcre2grep -M "(?s)\b$1 *\([[:alnum:]*,._\s\(\)-]*\);$";
 }
 
 libc_summ_c89()

> - Make libc_summ() standard-agnostics by passing in the filter
>   expression and the path to the doc as arguments.

It's really standards agnostic except for C89.  ISO C drafts
are pretty much all the same except for the original one, since
I was really ANSI C.  That's why C99 and C11 (and C2x if we add
it) can reuse libc_summ(), but C89 is an exception.

> - Make libc_summ() error out if the doc is not found.

The shell already errors out with a quite readable message:

$ ./bin/stdc c99 fgets
./bin/stdc: line 54: /usr/local/share/doc/c/c99/n1256.foo: No such file or directory
$ echo $?
1

Is it not good enough?

> - Give basic usage information on usage errors.

I prefer writing a man page.  It keeps the source code simpler.
Please keep this suggestion around, and resend it if you feel it's
necessary after having a man page (1 or 2 weeks maybe).

> - Make the standard version match case insensitive.

Okay.

> 
> Signed-off-by: Oskari Pirhonen <xxc3ncoredxx@gmail.com>

Please send individual patches for each of the logical changes.

> ---
> v1 -> v2:
> - Prefer pcre2grep from libpcre2. Suggested by Sam on IRC.
> 
>  bin/stdc | 101 +++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 68 insertions(+), 33 deletions(-)
> 
> diff --git a/bin/stdc b/bin/stdc
> index 8c725a2..0d322af 100755
> --- a/bin/stdc
> +++ b/bin/stdc
> @@ -1,65 +1,100 @@
> -#!/bin/bash
> +#!/usr/bin/env bash
>  
> -set -Eefuo pipefail;
> +set -Efuo pipefail

Why not set -e?  It's not documented in the commit message.

>  
> -prefix="/usr/local";
> -datarootdir="$prefix/share";
> -docdir="$datarootdir/doc";
> +prefix="/usr/local"
> +datarootdir="$prefix/share"
> +docdir="$datarootdir/doc"
> +
> +c89_filter='/A.3 LIBRARY SUMMARY/,$p'
> +c89_doc="$docdir/c/c89/c89-draft.txt"
> +c99_filter='/Library summary$/,/Sequence points$/p'
> +c99_doc="$docdir/c/c99/n1256.txt"
> +c11_filter='/Library summary$/,/Sequence points$/p'

This filter is really non_c89_filter :)

> +c11_doc="$docdir/c/c11/n1570.txt"
> +
> +pcregrep="$(type -P pcre2grep)"
> +if [[ -z "$pcregrep" ]]; then
> +	pcregrep="$(type -P pcregrep)"
> +fi

Not necessary anymore.  I switched to pcre2grep after your
suggestion.  I prefer not adding legacy fallbacks.  If someone
wants to backport this to an old system, that's not our problem.

>  
>  err()
>  {
> -	>&2 echo "$(basename "$0"): error: $*";
> -	exit 1;
> +	>&2 echo "$*"
>  }
>  
> -grep_proto()
> +die()
> +{
> +	err "$(basename "$0"): error: $*"
> +	exit 1
> +}
> +
> +# Args:
> +# 	1: usage error
> +usage()
>  {
> -	pcregrep -M "(?s)\b$1 *\([[:alnum:]*,._\s\(\)-]*\);$";
> +	err "$*"
> +	err
> +	err "usage: $(basename "$0") <version> <function>"
> +	err
> +	err "    version    ISO C version. Supported versions:"
> +	err "               C89, C99, C11"
> +	err "               (case insensitive)"
> +	err "    function   Function to look for"
> +	exit 1
>  }
>  
> -libc_summ_c89()
> +# Args:
> +# 	1: declaration to look for
> +grep_proto()
>  {
> -	sed -n '/A.3 LIBRARY SUMMARY/,$p' <"$docdir/c/c89/c89-draft.txt";
> +	"$pcregrep" -M "(?s)\b$1 *\([[:alnum:]*,._\s\(\)-]*\);\$"
>  }
>  
> +# Args:
> +# 	1: filter expression
> +# 	2: path to plaintext standard
>  libc_summ()
>  {
> -	sed -n '/Library summary$/,/Sequence points$/p';
> +	if [[ ! -r "$2" ]]; then
> +		die "cannot find or read '$2'"
> +	fi
> +	sed -n -e "$1" "$2"
>  }
>  
> +if [[ -z "$pcregrep" ]]; then
> +	die "pcre2grep or pcregrep required but is not installed"
> +fi
> +
>  case $# in
> -0)
> -	err "missing ISO C version.";
> -	;;
> -1)
> -	err "missing function name.";
> +0|1)
> +	usage "missing ISO C version and/or function name."
>  	;;
>  2)
>  	;;
>  *)
> -	shift;
> -	shift;
> -	err "unsupported extra argument(s): $*";
> +	shift
> +	shift
> +	usage "unsupported extra argument(s): ${*@Q}"
>  	;;
> -esac;
> +esac
>  
> -case "$1" in
> +case "${1@L}" in

I'm not a fan of shell features.  I prefer some command like
    "$(printf "%s" "$1" | tr [[:upper:]] [[:lower:]])"
which is much more readable (IMO).  Does that look good to you?

>  c89)
> -	shift;
> -	libc_summ_c89 \
> -	| grep_proto $@;
> +	libc_summ "$c89_filter" "$c89_doc" \
> +	| grep_proto "$2"

I applied the following:

commit 0e54f2590d9689c9b5cef2369d4664f4096fdd37 (HEAD -> main)
Author: Alejandro Colomar <alx@kernel.org>
Date:   Fri Mar 24 12:56:37 2023 +0100

    bin/: Remove some unnecessary shifts and `$@`s
    
    Reported-by: Oskari Pirhonen <xxc3ncoredxx@gmail.com>
    Signed-off-by: Alejandro Colomar <alx@kernel.org>

diff --git a/bin/stdc b/bin/stdc
index e70e067..1067cf8 100755
--- a/bin/stdc
+++ b/bin/stdc
@@ -45,19 +45,16 @@ esac;
 
 case "$1" in
 c89)
-       shift;
        libc_summ_c89 \
-       | grep_proto $@;
+       | grep_proto "$2";
        ;;
 c99)
-       shift;
        libc_summ <"$docdir/c/c99/n1256.txt" \
-       | grep_proto $@;
+       | grep_proto "$2";
        ;;
 c11)
-       shift;
        libc_summ <"$docdir/c/c11/n1570.txt" \
-       | grep_proto $@;
+       | grep_proto "$2";
        ;;
 *)
        err "$1: unsupported ISO C version.";

>  	;;
>  c99)
> -	shift;
> -	libc_summ <"$docdir/c/c99/n1256.txt" \
> -	| grep_proto $@;
> +	libc_summ "$c99_filter" "$c99_doc" \
> +	| grep_proto "$2"
>  	;;
>  c11)
> -	shift;
> -	libc_summ <"$docdir/c/c11/n1570.txt" \
> -	| grep_proto $@;
> +	libc_summ "$c11_filter" "$c11_doc" \
> +	| grep_proto "$2"
>  	;;
>  *)
> -	err "$1: unsupported ISO C version.";
> +	usage "$1: unsupported ISO C version."
>  	;;
> -esac;
> +esac
> +
> +# vim: set noexpandtab:

I'm not sure we want vim comments like this one for every file.  Aren't
they too noisy?  But maybe it's just me.  I use things like the following
for when I contribute regularly to projects that use a different rule for
indentation:

au bufnewfile,bufread ~/src/linux/man-pages/*/man*/* setlocal expandtab
au bufnewfile,bufread ~/src/linux/man-pages/*/man*/* setlocal shiftwidth=4
au bufnewfile,bufread ~/src/linux/man-pages/*/man*/* setlocal softtabstop=4
au bufnewfile,bufread ~/src/linux/man-pages/*/man*/* setlocal tabstop=5

au bufnewfile,bufread ~/src/nginx/* setlocal expandtab
au bufnewfile,bufread ~/src/nginx/* setlocal shiftwidth=4
au bufnewfile,bufread ~/src/nginx/* setlocal softtabstop=4
au bufnewfile,bufread ~/src/nginx/* setlocal tabstop=5

(tabstop=5 is for realizing when something is actually a tab, which for
example happens in Makefiles in NGINX.)


Thanks!

Cheers,
Alex


-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]