[Libguestfs] Re: factorization would be nice

[Richard W.M. Jones]

On Wed, Aug 12, 2009 at 12:09:25PM +0200, Jim Meyering wrote:
> Richard W.M. Jones wrote:
> > On Wed, Aug 12, 2009 at 09:15:31AM +0100, Richard W.M. Jones wrote:
> >> On Wed, Aug 12, 2009 at 10:07:16AM +0200, Jim Meyering wrote:
> >> >            List.iter (
> >> >              function
> >> >              | Pathname n ->
> >> >                  pr "  char *%s = args.%s;\n" n n;
> >> >                  pr "  ABS_PATH (%s, goto done);\n" n;
> >> >              | Device n ->
> >> >                  pr "  char *%s = args.%s;\n" n n;
> >> >                  pr "  RESOLVE_DEVICE (%s, goto done);" n;
> >> > 	     | Dev_or_Path n ->
> >
> > On thing to add here:
> >
> > The 'n's in this code are all different bindings.  So there's no way
> > to get a closure which captures the values of all 'n's.
> >
> > Thus in other code (but not in this case) you could have written:
> >
> >   let pr_args () =
> >     pr "  char *%s = args.%s\n" n n
> >   in
> >
> > where the value of 'n' would be captured in the closure 'pr_args', and
> > not have to be given explicitly each time.
> 
> Thanks for the explanation.
> Here's what I have now:
> 
>       (match snd style with
>        | [] -> ()
>        | args ->
>            pr "  memset (&args, 0, sizeof args);\n";
>            pr "\n";
>            pr "  if (!xdr_guestfs_%s_args (xdr_in, &args)) {\n" name;
>            pr "    reply_with_error (\"%%s: daemon failed to decode procedure arguments\", \"%s\");\n" name;
>            pr "    return;\n";
>            pr "  }\n";
> 	   let pr_args n =
> 	     pr "  char *%s = args.%s;\n" n n
> 	   in
>            List.iter (
>              function
>              | Pathname n ->
>                  pr_args n;
>                  pr "  ABS_PATH (%s, goto done);\n" n;
>              | Device n ->
>                  pr_args n;
>                  pr "  RESOLVE_DEVICE (%s, goto done);" n;
> 	     | Dev_or_Path n ->
>                  pr_args n;
>                  pr "  REQUIRE_ROOT_OR_RESOLVE_DEVICE (%s, goto done);" n;
>              | String n -> pr_args n
>              | OptString n -> pr "  %s = args.%s ? *args.%s : NULL;\n" n n n
>              | StringList n ->
>                  pr "  %s = realloc (args.%s.%s_val,\n" n n n;
>                  pr "                sizeof (char *) * (args.%s.%s_len+1));\n" n n;
>                  pr "  if (%s == NULL) {\n" n;
>                  pr "    reply_with_perror (\"realloc\");\n";
>                  pr "    goto done;\n";
>                  pr "  }\n";
>                  pr "  %s[args.%s.%s_len] = NULL;\n" n n n;
>                  pr "  args.%s.%s_val = %s;\n" n n n;
>              | Bool n -> pr "  %s = args.%s;\n" n n
>              | Int n -> pr "  %s = args.%s;\n" n n
>              | FileIn _ | FileOut _ -> ()
>            ) args;
>            pr "\n"
>       );

Looks fine.  You're only saving a few characters by factoring out
that function though.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html