Re: [PATCH v2] stdc: some improvements

[Oskari Pirhonen <xxc3ncoredxx@gmail.com>]

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