Re: [PATCH/RFA] spec functions

[Daniel Jacobowitz <drow at mvista dot com>]

On Fri, Nov 01, 2002 at 04:27:29PM -0800, Jason R Thorpe wrote:
> In reference to:
> 
> [1]	http://gcc.gnu.org/ml/gcc/2002-10/msg01622.html
> [2]	http://gcc.gnu.org/ml/gcc/2002-10/msg01624.html
> [3]	http://gcc.gnu.org/ml/gcc-patches/2002-10/msg01847.html
> 
> This patch adds support for spec functions.  Spec functions have the
> form:
> 
> 	%:function(args)

I love it.  Nits...

> + /* List of static spec functions.  */
> + 
> + #define INIT_STATIC_SPEC_FUNCTION(name, func) \
> + { NULL, name, func }
> + 
> + static struct spec_function static_spec_functions[] =
> + {
> +   INIT_STATIC_SPEC_FUNCTION ("if-exists", if_exists_spec_function),
> + };

I'm curious, why the macro?  A NULL isn't all that ugly...

> + /* Register a spec function.  */
> + 
> + void
> + register_spec_function (name, func)
> +      const char *name;
> +      char *(*func) PARAMS ((int, const char **));
> + {
> +   struct spec_function *sf;
> + 
> +   for (sf = spec_functions; sf != NULL; sf = sf->next)
> +     fatal ("duplicate `%s' spec function", name);

Surely you've lost a line or two!  Shouldn't there be a strcmp?

> +   processing_spec_function++;

Why do you disallow spec functions in spec function arguments?  It's
not terribly important, but it's unintuitive (and I didn't see it in
the docs either).

> + /* if-exists built-in spec function.
> + 
> +    Checks to see if the file specified in ARGS exists.  Returns
> +    the expanded pathname if found.  */
> + 
> + static char *
> + if_exists_spec_function (argc, argv)
> +      int argc;
> +      const char **argv;
> + {
> +   /* Must have only one argument.  */
> +   if (argc != 1)
> +     return NULL;
> + 
> +   /* We check for an absolute pathname because %s expansion returns
> +      the basename if the file is not found.  */
> + 
> +   if (IS_ABSOLUTE_PATHNAME (argv[0]) && ! access (argv[0], R_OK))
> +     return (xstrdup (argv[0]));
> + 
> +   return NULL;
>   }

Might want to comment that the usual use involves %s, and that it
doesn't actually do any expansion - it wasn't clear from the
comments/docs.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer