Re: [hackers] [slock][PATCH] Remove arg.h, simplify option parsing
Heyho,
Klemens Nanni wrote:
> >>> arg.h is really ugly code and way to complex for tools like slock that
> >>> have such minimal synopsis.
> >>
> >> it's a bit of "macro magic", but not really that complex if you think about
> >> it. Also, the usage within the code is dead simple and straightforward.
> >
> > I never called it magical or complex, it's just badly readable and I very
> > much prefer a cleaner version as proposed for obvious reasons.
>
> Well, I used the word complex actually. I meant that as in arg.h is too much
> code that does a simple job (parsing options specifically for slock) in a too
> complex way.
Well, arg.h seems has some complexity for sure when you read it the first time,
but I dare you to get used to it. This "complexity" has to be somewhere and I
rather have it in one file (UNIXy) used by many projects (more thorough testing
and auditing) than to rewrite this boring part of programm development over and
over again probably making more mistakes in the process than solving problems
with the actual program. arg.h was introduced to slock in 3bb868e4 and it was a
good change.
> >> Regardless of that, usage() does not have to be a function of it's own if
> >> it's called just once.
> >
> > Well, what's the problem with it being a standalone function?
>
> It's not needed, so why not calling die() directly instead of going through
> usage()? Code that does nothing is ought to be removed imho.
And the compiler will remove it. The code is maintained by humans, so we should
not optimize for machines. Compilers are good at that already. Having a
consistent way of handling the usage printing in many suckless projects makes it
easier on the humans maintaining the code and in some projects there are
multiple calls to usage() so therefore we have this one redundant call in the
hierarchy here.
Klemens Nanni wrote:
> This reduces the amount of strcmp() calls and comparisons in general to
> a minimum.
> …
> - } else if (!strcmp("--", argv[1])) {
> + } else if (argv[1][1] == '-' && argv[1][2] == '\0')
You should at least look at a few recent commits to a project before proposing
patches. If you would have done that, you would have surely stumbled upon the
second most recent commit c96e725 where we actually switched from char
comparisons to strcmp in other places of the code and you can also read the
reasoning behind this change in the commit message. So please make better
preparations when contributing to a project new to you and don't just dump all
your ideas to the maintainers.
--Markus
Re: [hackers] [slock][PATCH] Remove arg.h, simplify option parsing
On Sat, Oct 29, 2016 at 10:49:03PM +0200, Klemens Nanni wrote: arg.h is really ugly code and way to complex for tools like slock that have such minimal synopsis. it's a bit of "macro magic", but not really that complex if you think about it. Also, the usage within the code is dead simple and straightforward. I never called it magical or complex, it's just badly readable and I very much prefer a cleaner version as proposed for obvious reasons. Well, I used the word complex actually. I meant that as in arg.h is too much code that does a simple job (parsing options specifically for slock) in a too complex way.
Re: [hackers] [slock][PATCH] Remove arg.h, simplify option parsing
On Sat, Oct 29, 2016 at 08:39:07PM +0200, Laslo Hunhold wrote: On Fri, 28 Oct 2016 22:56:14 +0200 Klemens Nanni wrote: Hey Klemens, arg.h is really ugly code and way to complex for tools like slock that have such minimal synopsis. it's a bit of "macro magic", but not really that complex if you think about it. Also, the usage within the code is dead simple and straightforward. I never called it magical or complex, it's just badly readable and I very much prefer a cleaner version as proposed for obvious reasons. Regardless of that, usage() does not have to be a function of it's own if it's called just once. Well, what's the problem with it being a standalone function? It's not needed, so why not calling die() directly instead of going through usage()? Code that does nothing is ought to be removed imho. This (hopefully) is the final commit implementing proper command-line pargsing adhering to the POSIX Utility Syntax Guidelines[0] after submitting two incomplete patches to the hackers@suckless mailinglist. Just out of interest, what exactly is arg.h not adhering to with regard to these POSIX Utility Syntax Guidelines? I didn't mean arg.h isn't, that merely a statement about my patch alone since it didn't hold for the first version.
Re: [hackers] [slock][PATCH] Remove arg.h, simplify option parsing
On Fri, 28 Oct 2016 22:56:14 +0200 Klemens Nanni wrote: Hey Klemens, > arg.h is really ugly code and way to complex for tools like slock that > have such minimal synopsis. it's a bit of "macro magic", but not really that complex if you think about it. Also, the usage within the code is dead simple and straightforward. > Regardless of that, usage() does not have to be a function of it's own > if it's called just once. Well, what's the problem with it being a standalone function? > This (hopefully) is the final commit implementing proper command-line > pargsing adhering to the POSIX Utility Syntax Guidelines[0] after > submitting two incomplete patches to the hackers@suckless mailinglist. Just out of interest, what exactly is arg.h not adhering to with regard to these POSIX Utility Syntax Guidelines? > 0:http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02 Cheers Laslo -- Laslo Hunhold
Re: [hackers] [slock][PATCH] Remove arg.h, simplify option parsing
On 2016-10-28 23:56, Klemens Nanni wrote: arg.h is really ugly code and way to complex for tools like slock that have such minimal synopsis. it's not ugly, you just can't wrap your head around it, it's just a unix port of the plan 9 option parser[0] [0]: http://9p.io/sources/plan9/sys/include/libc.h --- () ascii ribbon campaign - against html e-mail /\ www.asciiribbon.org - against proprietary attachments
[hackers] [slock][PATCH] Remove arg.h, simplify option parsing
arg.h is really ugly code and way to complex for tools like slock that
have such minimal synopsis.
Regardless of that, usage() does not have to be a function of it's own
if it's called just once.
This (hopefully) is the final commit implementing proper command-line
pargsing adhering to the POSIX Utility Syntax Guidelines[0] after
submitting two incomplete patches to the hackers@suckless mailinglist.
Thanks to quinq and emg for pointing out this out.
0:
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02
---
arg.h | 65 -
slock.c | 32 ++--
2 files changed, 14 insertions(+), 83 deletions(-)
delete mode 100644 arg.h
diff --git a/arg.h b/arg.h
deleted file mode 100644
index 0b23c53..000
--- a/arg.h
+++ /dev/null
@@ -1,65 +0,0 @@
-/*
- * Copy me if you can.
- * by 20h
- */
-
-#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;\
- }\
- for (brk_ = 0, argv[0]++, argv_ = argv;\
- argv[0][0] && !brk_;\
- argv[0]++) {\
- if (argv_ != argv)\
- break;\
- argc_ = argv[0][0];\
- switch (argc_)
-
-/* Handles obsolete -NUM syntax */
-#define ARGNUM case '0':\
- case '1':\
- case '2':\
- case '3':\
- case '4':\
- case '5':\
- case '6':\
- case '7':\
- case '8':\
- case '9'
-
-#define ARGEND }\
- }
-
-#define ARGC() argc_
-
-#define ARGNUMF() (brk_ = 1, estrtonum(argv[0], 0, INT_MAX))
-
-#define EARGF(x) ((argv[0][1] == '\0' && argv[1] == NULL)?\
- ((x), abort(), (char *)0) :\
- (brk_ = 1, (argv[0][1] != '\0')?\
- (&argv[0][1]) :\
- (argc--, argv++, argv[0])))
-
-#define ARGF() ((argv[0][1] == '\0' && argv[1] == NULL)?\
- (char *)0 :\
- (brk_ = 1, (argv[0][1] != '\0')?\
- (&argv[0][1]) :\
- (argc--, argv++, argv[0])))
-
-#define LNGARG() &argv[0][0]
-
-#endif
diff --git a/slock.c b/slock.c
index ad539dc..283b04e 100644
--- a/slock.c
+++ b/slock.c
@@ -19,11 +19,8 @@
#include
#include
-#include "arg.h"
#include "util.h"
-char *argv0;
-
enum {
INIT,
INPUT,
@@ -289,14 +286,8 @@ lockscreen(Display *dpy, struct xrandr *rr, int screen)
return NULL;
}
-static void
-usage(void)
-{
- die("usage: slock [-v] [cmd [arg ...]]\n");
-}
-
int
-main(int argc, char **argv) {
+main(int argc, char *argv[]) {
struct xrandr rr;
struct lock **locks;
struct passwd *pwd;
@@ -307,13 +298,17 @@ main(int argc, char **argv) {
Display *dpy;
int s, nlocks, nscreens;
- ARGBEGIN {
- case 'v':
- fprintf(stderr, "slock-"VERSION"\n");
- return 0;
- default:
- usage();
- } ARGEND
+ if (argc > 1 && !strncmp("-", argv[1], 1)) {
+ if ((argc == 2 && !strcmp("-v", argv[1])) ||
+ (argc == 3 && !strcmp("-v", argv[1]) && !strcmp("--",
argv[2]))) {
+ fputs("slock-"VERSION"\n", stderr);
+ return 0;
+ } else if (!strcmp("--", argv[1])) {
+ --argc;
+ ++argv;
+ } else
+ die("usage: slock [-v] [cmd [arg ...]]\n");
+ }
/* validate drop-user and -group */
errno = 0;
@@ -367,13 +362,14 @@ main(int argc, char **argv) {
return 1;
/*
