automake
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: PATCH: patsubst support


From: Pavel Roskin
Subject: Re: PATCH: patsubst support
Date: Wed, 14 Feb 2001 18:54:37 -0500 (EST)

Hello, Alex!

Sorry for another delay. Your patch is very important, but unfortunately
I'm have been very busy recently.

> Here is an updated patsubst patch against CVS automake. Any patsubst
> style variables are now staticly expanded by automake, thus avoiding
> make portability problems.

We now have a single ChangeLog in the top-level directory.

> I have included tests for both the normal and conditional cases of
> variable expansion.

The test for the conditional case doesn't really test how the pattern is
expanded. And for me it's expanded incorrectly:

@address@hidden = @address@hidden doz

Note missing ".c" extentions.

The first test just fails.

> Please consider this for checkin.

Only when it's done correctly. Few notes about the implementation.

> +     (handle_options): add new option "no-expand-patsubst"

I don't like adding unsafe options. I cannot imagine any situation when
anybody would need it for a legitimate reason.

Using patterns with the older versions of Automake is incorrect, so I'm
not concerned if we break it.

It's better to have the same makefiles for users and developers unless
there are really serious reasons for them to be different.

But if you insist, the new options should be documented.

> +     (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. Add extra call to handle_options, otherwise options
> +     are set after they have effect.

Very dirty. Now we are calling handle_options twice.

> +sub expand_contents
> +{
> +    local ($var, $value, $cond) = @_;

We are using "my", not "local" for the new code. Eventually Automake will
get rid of "local" variables.

> +    local ($ret) = $value;
> +
> +    if ( $value =~ m/([^%]*)%([^%]*)%/ )
> +    {
> +     if ( $expand_patsubst )
> +     {
> +         local @curval = &variable_value_as_list ($var, $cond);

Maybe I'm missing something, but I don't see any error protection here.
You are working with user input. What if the expression is invalid?

> +$AUTOMAKE || exit 1
> +
> +grep 'address@hidden@' Makefile.in || exit 1
> +exit 0

As I said before, this is not a test for pattern expansion.

Regards,
Pavel Roskin




reply via email to

[Prev in Thread] Current Thread [Next in Thread]