[PATCH v2] stdc: some improvements

[PATCH v2] stdc: some improvements

[Oskari Pirhonen]

- Some small stylistic changes such as removing trailing semicolons and
  noisy `shift` calls.
- 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.
- Make libc_summ() standard-agnostics by passing in the filter
  expression and the path to the doc as arguments.
- Make libc_summ() error out if the doc is not found.
- Give basic usage information on usage errors.
- Make the standard version match case insensitive.

Signed-off-by: Oskari Pirhonen <xxc3ncoredxx@gmail.com>
---
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
 
-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'
+c11_doc="$docdir/c/c11/n1570.txt"
+
+pcregrep="$(type -P pcre2grep)"
+if [[ -z "$pcregrep" ]]; then
+	pcregrep="$(type -P pcregrep)"
+fi
 
 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
 c89)
-	shift;
-	libc_summ_c89 \
-	| grep_proto $@;
+	libc_summ "$c89_filter" "$c89_doc" \
+	| grep_proto "$2"
 	;;
 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:
-- 
2.39.2

Re: [PATCH v2] stdc: some improvements

[Alejandro Colomar]

[-- 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 --]

Re: [PATCH v2] stdc: some improvements

[Tom Schwindl]

> > +
> > +# 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:
>

I second this. Vim comments pollute source files and are way too editor
specific. It's also questionable if they're that useful. If someone sends
a patch with spaces instead of tabs, that's easy to catch and fix.

-- 
Best Regards,
Tom Schwindl

Re: [PATCH v2] stdc: some improvements

[Oskari Pirhonen]

[-- Attachment #1: Type: text/plain, Size: 4635 bytes --]

Hi,

On Fri, Mar 24, 2023 at 13:21:58 +0100, Alejandro Colomar wrote:
> 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.
> 

Style is obviously your call here.

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

... snip ...

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

That's good to know.

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

I suppose that is fine for me. I may be too used to having to performing
my own checks for files that I ignored Bash being able to do the same.

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

Heh. Perhaps you're biased towards man pages ;)

I'll keep it around (rebasing as needed for current and possibly future
edits).

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

Whoops. Will do in the future.

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

I forgot to mention that in the message, but it's no longer relevant
since you switched it to just pcre2grep. With `set -e` it was aborting
the script when `type -P` failed.

... snip ...

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

Fine by me.

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

... snip ...

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

This is an interesting idea that I've never considered before.

Thanks for at least accepting some of the suggestions.

- Oskari

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]