Re: [hackers] [PATCH] Update macros in arg.h to not cause hidden side effects to argv or rely on previously defined variables.

From: Mattias Andrée <maandree_AT_kth.se>
Date: Thu, 29 Aug 2024 00:24:20 +0200

ARGBEGIN...ARGEND is used to parse AND consume the options (and it also
consumes argv[0]), leaving the operands in argv for the code below ARGEND.
That's why mutates argv and argc. Now the code below ARGEND has no idea
which elements in argv are options and which elements are operands.


On Wed, 28 Aug 2024 18:44:53 -0300
sebastien peterson boudreau <sebastien.peterson.boudreau_AT_gmail.com> wrote:

> I mainly wanted to gauge interest in this change by sending it. I was
> looking at the macros in arg.h and comparing to the POSIX `getopt`, but
> what I disliked is that `ARGBEGIN` mutates `argv` and relies on the
> variable `argv0` being previously defined. Of course, this is also an
> issue with `getopt` using `optarg` IMO.
>
> This patch changes the macro to define a new variable, `_argv`,
> although the name is _certainly_ subject to change (especially
> considering we already have `argv_`. I just wanted to make the change
> quick and I figured others could bike-shed about the name). This allows
> the original `argv` argument to `main` to go unchanged. The only other
> change this requires is that `usage` take a string argument to print,
> which can be easily passed when called from `main`.
>
> I also went ahead and formatted the macro to be a bit easier to
> read just to make making the changes easier on myself. It looks like
> the origin author used tabs to vertically align, rather than spaces (as
> specified in the coding style), so the formatting breaks if you have a
> different tabwidth than them. I believe the formatting I used should
> look equally as correct, regardless of tabwidth.
>
> ---
> arg.h | 65 +++++++++++++++++++++++++++++------------------------------
> x.c | 25 +++++++++++------------
> 2 files changed, 44 insertions(+), 46 deletions(-)
>
> diff --git a/arg.h b/arg.h
> index a22e019..0726d0a 100644
> --- a/arg.h
> +++ b/arg.h
> _AT_@ -6,45 +6,44 @@
> #ifndef ARG_H__
> #define ARG_H__
>
> -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 \
> + char **_argv;\
> + argc--;\
> + for (_argv = argv+1;\
> + _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 ARGC() argc_
>
> -#define EARGF(x) ((argv[0][i_+1] == '\0' && argv[1] == NULL)?\
> +#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])))
> + (brk_ = 1, (_argv[0][i_+1] != '\0')?\
> + (&_argv[0][i_+1]) :\
> + (argc--, _argv++, _argv[0])))
>
> -#define ARGF() ((argv[0][i_+1] == '\0' && argv[1] == NULL)?\
> +#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])))
> + (brk_ = 1, (_argv[0][i_+1] != '\0')?\
> + (&_argv[0][i_+1]) :\
> + (argc--, _argv++, _argv[0])))
>
> #endif
> diff --git a/x.c b/x.c
> index d73152b..05a3767 100644
> --- a/x.c
> +++ b/x.c
> _AT_@ -15,7 +15,6 @@
> #include <X11/Xft/Xft.h>
> #include <X11/XKBlib.h>
>
> -char *argv0;
> #include "arg.h"
> #include "st.h"
> #include "win.h"
> _AT_@ -187,7 +186,7 @@ static char *kmap(KeySym, uint);
> static int match(uint, uint);
>
> static void run(void);
> -static void usage(void);
> +static void usage(char *);
>
> static void (*handler[LASTEvent])(XEvent *) = {
> [KeyPress] = kpress,
> _AT_@ -2024,7 +2023,7 @@ run(void)
> }
>
> void
> -usage(void)
> +usage(char *argv0)
> {
> die("usage: %s [-aiv] [-c class] [-f font] [-g geometry]"
> " [-n name] [-o file]\n"
> _AT_@ -2048,43 +2047,43 @@ main(int argc, char *argv[])
> allowaltscreen = 0;
> break;
> case 'c':
> - opt_class = EARGF(usage());
> + opt_class = EARGF(usage(argv[0]));
> break;
> case 'e':
> if (argc > 0)
> --argc, ++argv;
> goto run;
> case 'f':
> - opt_font = EARGF(usage());
> + opt_font = EARGF(usage(argv[0]));
> break;
> case 'g':
> - xw.gm = XParseGeometry(EARGF(usage()),
> + xw.gm = XParseGeometry(EARGF(usage(argv[0])),
> &xw.l, &xw.t, &cols, &rows);
> break;
> case 'i':
> xw.isfixed = 1;
> break;
> case 'o':
> - opt_io = EARGF(usage());
> + opt_io = EARGF(usage(argv[0]));
> break;
> case 'l':
> - opt_line = EARGF(usage());
> + opt_line = EARGF(usage(argv[0]));
> break;
> case 'n':
> - opt_name = EARGF(usage());
> + opt_name = EARGF(usage(argv[0]));
> break;
> case 't':
> case 'T':
> - opt_title = EARGF(usage());
> + opt_title = EARGF(usage(argv[0]));
> break;
> case 'w':
> - opt_embed = EARGF(usage());
> + opt_embed = EARGF(usage(argv[0]));
> break;
> case 'v':
> - die("%s " VERSION "\n", argv0);
> + die("%s " VERSION "\n", argv[0]);
> break;
> default:
> - usage();
> + usage(argv[0]);
> } ARGEND;
>
> run:
Received on Thu Aug 29 2024 - 00:24:20 CEST