Re: [hackers] [st][PATCH] arg.h: optimize & style

From: Nikita Zlobin <nick87720z_AT_gmail.com>
Date: Sun, 4 Jul 2021 15:48:51 +0500

In Sun, 4 Jul 2021 11:55:53 +0200
Hiltjo Posthuma <hiltjo_AT_codemadness.org> wrote:

> On Sun, Jul 04, 2021 at 12:30:27PM +0500, Nikita Zlobin wrote:
> > - improved code readability
> > - cleaned redundant variables and checks
> > - isolated reusable part for (E)ARGF
> > ---
> > arg.h | 65
> > +++++++++++++++++++++++++++++------------------------------ 1 file
> > changed, 32 insertions(+), 33 deletions(-)
> >
> > diff --git a/arg.h b/arg.h
> > index a22e019..ee0bbc6 100644
> > --- a/arg.h
> > +++ b/arg.h
> > _AT_@ -9,42 +9,41 @@
> > extern char *argv0;
> >
> > /* use main(int argc, char *argv[]) */
> > -#define ARGBEGIN for (argv0 = *argv, argv++, argc--;\
> > - argv[0] && argv[0][0] ==
> > '-'\
> > - && argv[0][1];\
> > - argc--, argv++) {\
> > - char argc_;\
> > - char **argv_;\
> > - int brk_;\
> > - if (argv[0][1] == '-' &&
> > argv[0][2] == '\0') {\
> > - argv++;\
> > - argc--;\
> > - break;\
> > - }\
> > - int i_;\
> > - for (i_ = 1, brk_ = 0, argv_ =
> > argv;\
> > - argv[0][i_] &&
> > !brk_;\
> > - i_++) {\
> > - if (argv_ != argv)\
> > - break;\
> > - argc_ = argv[0][i_];\
> > - switch (argc_)
> > -
> > -#define ARGEND }\
> > +#define ARGBEGIN \
> > + for (argv0 = *argv, argv++, argc--;
> > \
> > + argv[0] && argv[0][0] == '-' &&
> > argv[0][1]; \
> > + argc--, argv++ )
> > \
> > + {
> > \
> > + char arg_;
> > \
> > + int brk_;
> > \
> > + if (argv[0][1] == '-' &&
> > argv[0][2] == '\0') \
> > + {
> > \
> > + argv++;
> > \
> > + argc--;
> > \
> > + break;
> > \
> > + }
> > \
> > + for (brk_ = 0, argv[0]++;
> > \
> > + !brk_ && (arg_ = argv[0][0]);
> > \
> > + argv[0]++ )
> > \
> > + {
> > \
> > + switch (arg_)
> > +#define ARGEND \
> > + } \
> > }
> >
> > -#define ARGC() argc_
> > +#define _ARGF(x) \
> > + ( (argv[0][1] == '\0' && argv[1] == NULL)
> > \
> > + ? (x)
> > \
> > + : (brk_ = 1, ( (++argv[0]) [0] != '\0')
> > \
> > + ? argv[0]
> > \
> > + : ( argc--, (++argv)[0] )
> > \
> > + )
> > \
> > + )
> >
> > -#define EARGF(x) ((argv[0][i_+1] == '\0' && argv[1] ==
> > NULL)?\
> > - ((x), abort(), (char *)0) :\
> > - (brk_ = 1, (argv[0][i_+1] !=
> > '\0')?\
> > - (&argv[0][i_+1]) :\
> > - (argc--, argv++, argv[0])))
> > +#define ARGC() arg_
> >
> > -#define ARGF() ((argv[0][i_+1] == '\0' && argv[1]
> > == NULL)?\
> > - (char *)0 :\
> > - (brk_ = 1, (argv[0][i_+1] !=
> > '\0')?\
> > - (&argv[0][i_+1]) :\
> > - (argc--, argv++, argv[0])))
> > +#define EARGF(x) (_ARGF( ((x), abort(), NULL) ))
> > +
> > +#define ARGF() (_ARGF( NULL ))
> >
> > #endif
> > --
> > 2.31.1
> >
> >
>
> Hi,
>
> Thanks, but I prefer the current style one.
>
> I'm not confident this patch doesn't modify any behaviour.
> For example I see the `i` variable was removed, but it is actually
> important to not modify argv as this causes issues on NetBSD and
> OpenBSD process listing (see commit
> a5a928bfc1dd049780a45e072cb4ee42de7219bf).
>
> This is just one example. Unless it fixes a bug I rather keep the
> current code.
>

st and tabbed indeed have slightly different arg.h - 20h edited it
after copying from st. There I simply copied my version from tabbed to
st. Imho, it really needs to be in shared collections (proposed at dev).

As for 'i_' - I checked version from git tabbed, and it's arg.h has no
such variable.

About style - project guidelines specify, that most important is its
readability. I will try to point to issues, I tried to solve:

- '{' in 2 for loops better be at new line to better split multiline
  condition from body.
- This separation is not so necessary with one-line condition, but then
  if (cond) line is considered part of main level, even without
  emptyline splitting it from previous code, so it's rather level
  separation (well, this is my taste, readability is still ok if '{' is
  not at newline).
- miltiline ?: operator: placing '?:' in line start helps better see
  split between different operator operands.
- '?:' operands at newlines after better perceived as part of '?:' if
  at least aligned with condition, if not indented. There they look
  more on their own.
- (type *)0 vs NULL: I found no advantages to prefer first form, while
  searching. Answers say, that in some arch NULL may be not even 0, but
  garbage. As for me, first form is nothing but minus for readability.
Received on Sun Jul 04 2021 - 12:48:51 CEST