Re: [hackers] [slock][PATCH] Remove arg.h, simplify option parsing

From: Markus Teich <markus.teich_AT_stusta.mhn.de>
Date: Mon, 31 Oct 2016 12:32:54 +0100

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
Received on Mon Oct 31 2016 - 12:32:54 CET