automake-patches
[Top][All Lists]
Advanced

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

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


From: Ralf Wildenhues
Subject: Re: [PATCH 4/9] Warnings win over strictness in AM_INIT_AUTOMAKE.
Date: Wed, 12 Jan 2011 19:54:21 +0100
User-agent: Mutt/1.5.20 (2010-08-04)

Here comes my (belated, as always) re-review; I am not repeating nits
already dealt with before.

* Stefano Lattarini wrote on Wed, Jan 05, 2011 at 08:22:13PM CET:
> On Wednesday 05 January 2011, Ralf Wildenhues wrote:
> > * 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.

> > > 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.

> > > * 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

> > > @@ -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
> > > +  # 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.

But that's exactly what I mean: you need patch 7/9 precisely because
you have changed the requirements that you push onto callers of
process_*options_list.  Let me show you what I mean: with your patch
series, you would also need something like the following:

To this POD text in Options.pm:

| =item C<process_option_list ($where, @options)>
| 
| =item C<process_global_option_list ($where, @options)>
| 
| 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>.

you would need to add the following:

  These functions should be called at most once for each set of options
  having the same precedence; i.e., do not call it twice for two options
  from C<AM_INIT_AUTOMAKE>.

It is important for maintainability that the POD documentation is
correct.

And actually, now that we see the requirement printed, it is clear that
this is not a good move: the API has suddenly become less easy to use,
because it is easier for the callers of process_*option_list to use it
wrongly.  http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html

One better API would be one that either
- checks that options of the same precedence are not split up when
  passed, or
- computes the set of active warnings only strictly after all options
  (of a certain precedence) have been processed.


And upon rereading the code, it is also quite clear where the other
reported precedence bugs come from: the global_options list is abused
for two sets of options: those from AM_INIT_AUTOMAKE, which should have
lower precedence than AUTOMAKE_OPTIONS, and those from the command line,
which should have higher precedence than AUTOMAKE_OPTIONS.  The early
conflation made correct precedence semantics impossible.  The right way
here would be to have three stores of options.

And now that also makes it clear why the automake code printed some of
the options as arguments in the rebuild rule for Makefile.in: because
precedence was botched, that was needed in order to avoid these options
to get lost.  Or so at least I believe.

Hmm, set_strictness has some of the same problems, except one
complication is that some warnings may apply already at global level,
before any Makefile.am file is even opened.  I'm guessing three sets are
needed here too.


So, now with that said, I'm not sure whether I should approve this
patch.  What do you think?

> >  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 agree that it should be, but this, too, should be documented (in
autoconf.texi and maybe also NEWS) and tested, when it works.

> > 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.

Well, as an alternative for the requirement above would be to only
process warnings in a separate function compute_warning_set, once
all options coming from a given source (AM_INIT_AUTOMAKE) have been set.
That would be a bit more flexible.


> > 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).

See above.

> > 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?)

See above.

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

Cheers,
Ralf



reply via email to

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