Re: [PATCH 6/9] Change signature of 'Automake::Options::_process_option_list()'.

[Ralf Wildenhues]

* Stefano Lattarini wrote on Tue, Jan 04, 2011 at 06:48:06PM CET:
> Change signature of 'Automake::Options::_process_option_list()'.
> 
> This only modifies internal details in the automake implementation,
> bearing no externally visible effect, but preparing the way for the
> final fix of Automake bug#7669 a.k.a. PR/547.
> 
> * lib/Automake/Options.pm (_process_option_list): Now accepts as

s/Now accepts/Accept/

> arguments a list of hash references with keys 'option' and 'where',
> where 'option' is an option as might occur in AUTOMAKE_OPTIONS or
> M_INIT_AUTOMAKE, and 'where' is the location where that occurred.

s/^/A/
s/that/it/

> (process_option_list, process_global_option_list): Update.
> * automake.in (handle_options, scan_autoconf_traces): Update.


> --- a/lib/Automake/Options.pm
> +++ b/lib/Automake/Options.pm
> @@ -222,30 +222,35 @@ sub unset_global_option ($)
>  }
>  
>  
> -=item C<process_option_list ($where, @options)>
> +=item C<process_option_list (@options)>

The placeholder variable names here should match those actually used in
the perl code inside the function, i.e., @list, to make it unambiguous
wrt.  _process_option_list.

> -=item C<process_global_option_list ($where, @options)>
> +=item C<process_global_option_list (@options)>

Likewise.

> -Process Automake's option lists.  C<@options> should be a list of
> -words, as they occur in C<AUTOMAKE_OPTIONS> or C<AM_INIT_AUTOMAKE>.
> +Process Automake's option lists.  C<@options> should be a list of hash
> +references with keys C<option> and C<where>, where C<option> is an option
> +as might occur in C<AUTOMAKE_OPTIONS> or C<AM_INIT_AUTOMAKE>, and

s/might/they/

> +C<where> is the location where that option occurred.
>  
>  Return 1 on error, 0 otherwise.
>  
>  =cut
>  
>  # $BOOL
> -# _process_option_list (\%OPTIONS, $WHERE, @OPTIONS)
> -# --------------------------------------------------
> -# Process a list of options.  Return 1 on error, 0 otherwise.
> -# \%OPTIONS is the hash to fill with options data, $WHERE is
> -# the location where @OPTIONS occurred.
> -sub _process_option_list (\%$@)
> +# _process_option_list (\%OPTIONS, @OPTIONS)

The placeholder should be the upper-cased version of the variable name
used, i.e., @LIST.

> +# ------------------------------------------
> +# Process a list of options.  \%OPTIONS is the hash to fill with
> +# options data.
> +# options as get passed to public subroutines process_option_list() and
> +# process_global_option_list().

These sentences are botched.  Please fix.  Also, the documentation above
requires the changes discussed in the review of 4/9.

> +sub _process_option_list (\%@)
>  {
> -  my ($options, $where, @list) = @_;
> +  my ($options, @list) = @_;
>    my @warnings = ();
>  
> -  foreach (@list)
> +  foreach my $h (@list)
>      {
> +      my $_ = $h->{option};
> +      my $where = $h->{where};

Quoting of literal strings inside {}?

>        $options->{$_} = $where;
>        if ($_ eq 'gnits' || $_ eq 'gnu' || $_ eq 'foreign')
>       {
> @@ -314,7 +319,8 @@ sub _process_option_list (\%$@)
>       }
>        elsif (/^(?:--warnings=|-W)(.*)$/)
>       {
> -       push @warnings, split (',', $1);
> +       my @w = map { { cat => $_, loc => $where} } split (',', $1);
> +       push @warnings, @w;
>       }
>        else
>       {
> @@ -326,24 +332,22 @@ sub _process_option_list (\%$@)
>    # We process warnings here, so that any explicitly-given warning setting
>    # will take precedence over warning settings defined implicitly by the
>    # strictness.
> -  foreach my $cat (@warnings)
> +  foreach my $w (@warnings)
>      {
> -      msg 'unsupported', $where, "unknown warning category `$cat'"
> -     if switch_warning $cat;
> +      msg 'unsupported', $w->{loc}, "unknown warning category `$w->{cat}'"
> +     if switch_warning $w->{cat};

see above

>      }
>    return 0;
>  }
>  
> -sub process_option_list ($@)
> +sub process_option_list (@)
>  {
> -  my ($where, @list) = @_;
> -  return _process_option_list (%_options, $where, @list);
> +  return _process_option_list (%_options, @_);
>  }
>  
> -sub process_global_option_list ($@)
> +sub process_global_option_list (@)
>  {
> -  my ($where, @list) = @_;
> -  return _process_option_list (%_global_options, $where, @list);
> +  return _process_option_list (%_global_options, @_);
>  }
>  
>  =item C<set_strictness ($name)>

OK with that fixed.

Thanks,
Ralf