[Toybox] ifconfig: error helper?

[Rob Landley]

On 04/13/2013 07:56:34 PM, Isaac Dunham wrote:
> Hello,
> I took a look at ifconfig to see about show_help vs toys.exithelp,  
> and I saw this repeated 16 times ("string" is the only thing that  
> changes):
> } else if (!strcmp(*argv, "string")) {
>   if(*++argv == NULL) {
>     errno = EINVAL;
>     show_help();
>   }
>   set_string(...)
> 
> This looks like a pretty obvious candidate for a helper function that  
> does
> more than show help and exit.

Indeed.

> It would seem one could do something like this:
> void nullarg_help(char * args)
> {
>   if (*args == NULL) {
>     errno = EINVAL;
>     toys.exithelp++;
>     error_exit("missing argument");
>   }
> }
> ..
> } else if (!strcmp(*argv, "string")) {
>   nullarg_help(*++argv);
>   set_string(...)

I was actually thinking something table driven, trying to incorporate  
the actual work the option did into the table (possibly with a function  
pointer as one of the table entries), but hadn't gone through and  
triaged all the cases yet.

> 
> I probably got the pointers wrong, but that should show the general  
> idea.
> It will drop 40 lines (16 occurrances * 3 lines saved - 8 lines to  
> define).
> 
> Any thoughts on this?

You've definitely got the right idea, I was just thinking of taking it  
further. I'll try to code something up in the morning to show what I  
had in mind.

(On the other hand, the changes you're suggesting wouldn't stop me from  
doing the changes I'm thinking of on top of them. If you'd inclued a  
patch I'd probably apply it now.)

In general, I try to do the simple cleanups first to get the code  
compact enough I can wrap my head around it and do design changes once  
I've run out of landscaping work. At some point I need to read the  
entire file start to finish and understand what it's _doing_. I haven't  
sat down and done that yet. When I try, I keep finding... things.

Rob