Re: [patch] give gpioctl.c some love

[Timo Buhrmester <fstd.lkml%gmail.com@localhost>]

> Imo, this patch does not solve a problem, but creates new ones, maybe...
I wonder what problems you imagine that to be. Care to elaborate?

>  The constness that you sprinkle is definitely not needed, only makes
> the code cluttered imo.
It actually makes it easier to understand because it's explicit that a function will not modify the data pointed to by a pointer to something const.
This isn't a problem for you, since being the original author you already know how the code works. Someone trying to read and understand the code in a readonable timeframe does not have this implicit knowledge.


> > -static void gpioread(int, char *);
> > -static void gpiowrite(int, char *, int);
> > -static void gpioset(int pin, char *name, int flags, char *alias);
> > -static void gpiounset(int pin, char *name);
> > -static void devattach(char *, int, uint32_t, uint32_t);
> > +static void gpioread(int pin, const char *gp_name);
> > +static void gpiowrite(int pin, const char *gp_name, int value);
> > +static void gpioset(int pin, const char *name, int flags, const char *alias);
> > +static void gpiounset(int pin, const char *name);
> This is wrong.  We don't use variable names in function prototypes for
> good reason.
> > -static void gpioset(int pin, char *name, int flags, char *alias);
> > -static void gpiounset(int pin, char *name);
What is that good reason, and why did you do name the parameters for some prototypes but not for others, then?


> What does it gain you to add an empty line here?
Probably just a habit, I tend to do that before the function definitions follow.  Feel free to disregard


> > -	} else {
> > -		if (ioctl(devfd, GPIOTOGGLE, &req) == -1)
> > -			err(EXIT_FAILURE, "GPIOTOGGLE");
> > -	}
> > +	} else if (ioctl(devfd, GPIOTOGGLE, &req) == -1)
> > +		err(EXIT_FAILURE, "GPIOTOGGLE");
> >  
> >  	if (state)
> >  		printf("%d\n", value < 2 ? value : 1 - req.gp_value);
> > @@ -277,19 +244,19 @@ gpiowrite(int pin, char *gp_name, int value)
> >  }
> >  
> 
> I don't see an advantage of just rearranging the code.
I didn't see an advantage of doing
|  if (..) {
| 	...
| } else {
| 	if (...)
| 		statement;
| }
When there's already a convention established for this.  Technically, it declutters the code by two lines.


> > -	if (name != NULL)
> > +	if (name)
> technically correct, but I prefer the expressivenes of the original
> statement.
I guess that's a matter of taste.


> > -	const char *progname;
> > -
> > -	progname = getprogname();
> > -	fprintf(stderr, "usage: %s [-qs] device [pin] [0 | 1 | 2 | "
> > -	    "on | off | toggle]\n", progname);
> > -	fprintf(stderr, "       %s [-q] device pin set [flags] [name]\n",
> > -	    progname);
> > -	fprintf(stderr, "       %s [-q] device pin unset\n", progname);
> > -	fprintf(stderr, "       %s [-q] device attach device offset mask "
> > -	    "[flag]\n",
> > -	    progname);
> > +	const char *pn = getprogname();
> > +
> > +	#define U(...) fprintf(stderr, __VA_ARGS__)
> > +	U("usage: %s [-qs] device [pin] [0 | 1 | 2 | on | off | toggle]\n", pn);
> > +	U("       %s [-q] device pin set [flags] [name]\n", pn);
> > +	U("       %s [-q] device pin unset\n", pn);
> > +	U("       %s [-q] device attach device offset mask [flag]\n", pn);
> > +	#undef U
> 
> This is ugly to say the leas[t], please, no, no, no...
It's not like I'm happy with it, but I'm not too sure how what it tries to replace is anywhere less ugly.
If you're concerned about variadic macros not being a standard C feature; they are, as of C99


> In conclusion, I don't like your patch, and since same/similar code is
> used in other BSDs, I think we should not apply it.
Thanks for the comments, but you were only commenting on minor points, when the primary cruft the patch removes is the code being literally right-aligned, as in the following 3 (three) lines:
-					for (bs = pinflags; bs->string != NULL;
-					     bs++) {
-						if (!strcmp(argv[n],
-						    bs->string)) {
[...]
-						errx(EXIT_FAILURE,
-						    "%s: invalid value",
-						    argv[2]);
Note the deep indentation at least.  It is difficult to read, the deep nesting requires considerable state to be kept mentally when following the code.
I think it's pretty awful, especially in the light of the nesting not even being required here.

So even if you disagree on the details, I'd like to hear your take on that (that latter) (and ideally from someone who didn't happen to wrote this code, too)


Timo