Re: [PATCH 4/9] Warnings win over strictness in AM_INIT_AUTOMAKE.

[Stefano Lattarini]

On Wednesday 05 January 2011, Ralf Wildenhues wrote:
> Hi Stefano,
> 
> * Stefano Lattarini wrote on Tue, Jan 04, 2011 at 06:38:04PM CET:
> > This is derived from [PATCH 07/10] of the older series.
> > It requires a review.
> 
> Thank you for rewriting the patch.
>
Really you should thank "git rebase -i" ;-)

> See below for comments.
> 
> > Warnings win over strictness in AM_INIT_AUTOMAKE.
> > 
> > This change ensures that, for what concerns the options specified
> > in AM_INIT_AUTOMAKE,  explicitly-defined warnings always take
> > precedence over implicit strictness-implied warnings.  Related to
> > Automake bug#7669 a.k.a. PR/547.
> 
> PR automake/547
>
Will fix.

> > * lib/Automake/Options.pm (_process_option_list): Parse explicit
> > warnings only after the strictness level has been set.
> > * tests/warnings-win-over-strictness.test: Extend.
> 
> > --- a/lib/Automake/Options.pm
> > +++ b/lib/Automake/Options.pm
> > @@ -242,6 +242,7 @@ Return 1 on error, 0 otherwise.
> >  sub _process_option_list (\%$@)
> >  {
> >    my ($options, $where, @list) = @_;
> > +  my @warnings = ();
> >  
> >    foreach (@list)
> >      {
> > @@ -313,11 +314,7 @@ sub _process_option_list (\%$@)
> >     }
> >        elsif (/^(?:--warnings=|-W)(.*)$/)
> >     {
> > -     foreach my $cat (split (',', $1))
> > -       {
> > -         msg 'unsupported', $where, "unknown warning category `$cat'"
> > -           if switch_warning $cat;
> > -       }
> > +     push @warnings, split (',', $1);
> >     }
> >        else
> >     {
> > @@ -326,6 +323,14 @@ sub _process_option_list (\%$@)
> >       return 1;
> >     }
> >      }
> > +  # We process warnings here, so that any explicitly-given warning setting
> 
> s/We p/P/
> 
> > +  # will take precedence over warning settings defined implicitly by the
> > +  # strictness.
> 
> Well, this works in the current code base, but only by accident: namely,
> only because process_option_list is only ever called once, and with all
> options at once.
>
Hmm... no, it's potentially called many times in `handle_options()'.
But the later [PATCH 7/9] takes care of this.

>  If some code later calls it like
>   process_option_list (first-set-of-options);
>   process_option_list (second-set-of-options);
> 
> then things will go wrong again.  I suspect that it will mean that
>   AM_INIT_AUTOMAKE([foreign -Wno-portability])
>   AUTOMAKE_OPTIONS = gnu
> 
> won't do what we want.  Hmm.  What exactly is it that we want to happen
> in this case?  Should gnu override -Wno-portability if specified in a
> (to-be) higher order place?
>
I assumed without saying that yes, this was to be the intended behaviour.
And I still think it should be.  Sorry for not having been explicit about
that before.

> I see two ways out: warnings are only switched after all options are
> processed.
>
This is not good IMO, as it breaks usages like the the one in your
example above.

> Or: process_option_list is documented to require *all*
> options.  The latter seems fragile.
>
Still, currently this is the best option IMVHO.  And in fact, the later
[PATCH 7/9] works exactly by ensuring that `handle_options()' calls
`process_option_list()' only once (this is the reason the preliminary
PATCH [6/9] "Change signature of Automake::Options::_process_option_list"
is required).

> Either way though, we need a consistent definition (and documentation)
> of intended semantics, both internally of process_option_list and user
> side of option handling semantics.
>
Agreed on the user-side documentation (which can be done in a follow-up
patch though).  But I don't think that internal documentation is really
required; nor can I think about how to formulate it (suggestions?)

> > +  foreach my $cat (@warnings)
> > +    {
> > +      msg 'unsupported', $where, "unknown warning category `$cat'"
> > +   if switch_warning $cat;
> > +    }
> >    return 0;
> >  }
> 
> Thanks,
> Ralf
> 

Regards,
  Stefano