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
