Re: PATCH: patsubst support

[Pavel Roskin]

Hello, Alex!

> Here is a new version of the patsubst patch against cvs HEAD.

Thanks! Were are getting closer.

> +     * automake.in (expand_contents): add new function to perform
> +     the patsubst expansion
> +     (value_to_list): add support for patsubst style variable
> +     substitution.
> +     (read_main_am_file): call expand_contents to output
> +     variables.
> +
> +     * tests/patsubst.test: add test for patsubst expansion
> +
> +     * tests/patsubst2.test: add test for conditional patsubst
> +     expansion
> +
> +     * tests/Makefile.am: reference patsubst.test and patsubst2.test

Sorry, I'll be pedantic. Capitalize the first word (s/add/Add/). Don't
leave blank lines - they are used to distinguish separate commits.  Don't
omit a full stop.

> -             ($from = $2) =~ s/(\W)/\\$1/g;
> +             ($from = $2) =~ s/(\W)/$1/g;

I don't understrand this. This change will affect the traditional rules as
well. It should probably be a separate patch if it fixes a separate issue.
You may even need a test case.

> -         if ($from)
> +         if ($from =~ '^([^%]*)%([^%]*)')
>           {
> -             grep (s/$from$/$to/, @temp_list);
> +             # patsubst style substitution
> +             my ($prefrom, $suffrom, $preto, $sufto);
> +             $prefrom = $1;
> +             $suffrom = $2;

When I first saw "preto" and "suffrom" I thought it's probably in Latin
:-)

Bytes are cheap, especially when they don't end up in every Makefile.in
for every package. Use suffix_from or whatever is appropriate.

...
> +         }
> +         elsif ($from)
> +         {
> +             # standard substitution reference style
> +             grep (s/$from$/$to/, @temp_list);
>           }

what I don't see here is a case for $from containing '%' but not matching
our rule. It should be something like:

         elsif ($from)
         {
             # standard substitution reference style
             if ($from =~ '%')
             {
                 &am_error ("\`$from' cannot be expanded");
             }
             grep (s/$from$/$to/, @temp_list);
         }

Note that am_error() doesn't stop the execution, but causes automake to
exit with code 1.

> +sub expand_contents
> +{
> +    my ($var, $value, $cond) = @_;
> +    my ($ret) = $value;
> +
> +    if ( $value =~ m/([^%]*)%([^%]*)%/ )
> +    {
> +     my @curval = &variable_value_as_list ($var, $cond);
> +     my ( $val );
> +     $ret = '';
> +     foreach $val ( @curval )
> +     {
> +         $ret .= $val . " ";
> +     }

How about this:

join(" ", @curval)

Everything else is fine. I'm sorry that I'm keeping bouncing your work,
but the Automake code should be of the highest quality.

Regards,
Pavel Roskin