Re: [Xen-devel] [PATCH 1/8] Add core.sh and wrapper function

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 04/10/2015 03:29 PM, Stefano Stabellini wrote:
>> +arg_parse_cmd=\
>> +"local -a args;
>> +local _a;
>> +local _vn;
>> +local _m;
>> +
>> +_m=true;
>> +
>> +for _a in \"\$@\" ; do
>> +    false && echo \"Evaluating \${_a} [[ \"\${_a/=}\" = \"\${_a}\" ]]\";
>> +    if \$_m && [[ \"\${_a/=}\" != \"\${_a}\" ]] ; then
>> +        false && echo Parameter;
>> +        _vn=\${_a%%=*};
>> +        eval \"local \$_vn\";
>> +        eval \"\$_a\";
>> +    elif \$_m && [[ \"\${_a}\" == \"--\" ]] ; then
>> +        false && echo Separator;
>> +        _m=false;
>> +    else
>> +        false && echo Argument;
>> +        _m=false;
>> +        args+=(\"\$_a\");
>> +    fi;
>> +done"
> 
> I am sorry but I cannot bear myself to introduce so much complexity into
> the world.  raisin is supposed to be easy to read.  If we end up
> actually needing something so powerful and so complex in the future we
> can import it then, but I think that at the moment we can do nicely
> without it.

I can certainly understand the sentiment, but the idea behind this
calling convention is actually to make it *easier* both to read and
debug most of the source file, at the cost of two very complex, but
well-tested and generic macros ($arg_parse and $requireargs).

Consider some criteria:
- Difficulty of writing or reading code that calls a function
- Difficulty of writing a function which will be called
- Availability of sensible defaults
- Ability to make sure all necessary arguments have been specified
- Passing function arguments into sub-functions
- Global variables are nasty to debug

Consider alternate ways of passing arguments into functions:

1. Positional

i.e., "cp SRC DEST"

In this model, it's easy to make sure that all necessary arguments have
been specified; and there are no nasty global variables to work with.

However is really hard to program and read because you have to remember
what argument goes where.  You also have to specify all possible
arguments -- you can't have a default.  And if you need to take an
argument and pass it to another function you call, you have to repeat it
again.

2. Option

i.e., "-v --components='a b'"

This is easier to read if you use long options (or common ones like -v
or -y), and you can also have defaults.  Additionally, you can have a
certain amount of error checking -- if you get an argument you don't
recognize you can throw an error.

But you have to write a lot of boilerplate option processing at the top
of each function; and if you have anything a bit complex (line
"--components= above"), it makes it a lot harder to read.  And you have
to write new code every time, giving you more opportunity for bugs.

Consider the argument parsing code you have in raise at the moment -- it
assumes that you will run a command without any additional parameters.
What if you wanted to do something like this:

"raise -v build xen"

The current code would have to be refactored.

And you have to write a lot of custom code to check to make sure that
all the required arguments have been passed in.

And you have the same problem that if you need to pass an argument
further down the call stack, you have to repeat it again.

3. Global variables

i.e.,
"VAR=foo
 function"

This is a bit easier to read in the sense that you're using full
variable names for options.  It's easier to write, since you don't have
any parsing code.  Additionally, variables can be passed further down
the calling stack automatically without having to be explicitly copied
around.  In theory you should be able to have sensible local defaults.

But you have all the problems with global variables being really hard to
debug.

4. Prefix variable

i.e., "YES=y check-builddep"

This is a bit easier to read in the sense that you're using full
variable names for options.  It's easier to write, since you don't have
any parsing code.  Additionally, variables can be passed further down
the calling stack automatically without having to be explicitly copied
around.  In theory you should be able to have sensible local defaults.

I hadn't thought about this as a calling convention.  It certainly has
the advantage that it's build into bash and fairly well-understood by
experienced users.

But I do think it's a bit unnatural to have to prefix all the variables
to the function.

5. VAR=VAL automatic parsing with inheritance

This allows you to have a rich, consistent way to pass in arguments that
are descriptive.  It's easy to write code to parse the arguments and
check to make sure you have all the ones you need: Just add $arg_parse
and $requireargs at the top of your function.  They are automatically
declared local, so there's no worry about polluting the global
namespace.  They are automatically inherited, so you don't have to keep
passing information further down the stack.  And you can have sensible
defaults.

I think #5 is well worth the little bit of macro hackery confined to a
handful of well-tested macros, particularly if raise becomes a fairly
large and fully-functional script.  I've been using this calling
convention for several years now in my own scripts, so it's pretty well
tested code-wise, feature-wise, and it hasn't become a cancer that
spreads throughout the rest of the code. :-)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel