Re: [PATCH] autoheader: check templates of all config headers

[Warren Young]

On Oct 9, 2015, at 10:48 PM, Paul Eggert <address@hidden> wrote:
> 
> The basic idea for this sounds good; thanks.  It'd be nice if someone else 
> who uses Perl more than I could look over the details.

Well, I know Perl, but I don’t know the autotools internals, so all I can do is 
nitpick style details.  If that’s all you want, then:

> sub templates_for_header ($)

Don’t use subroutine prototypes.  They don’t do what you want, so perlcritic 
rightly complains about them:

  http://goo.gl/bDCsJ4

I suggest running the changed file through perlcritic at least at its default 
level.  I try to meet --harsh level in my code with minimal “## no critic” 
overrides.

> return @templates if (@templates);

Just a nit, but I typically leave the parens off when using the postfix form of 
“if”, especially when the expression is simple, as in this case.

> my $in = new Autom4te::XFile ("< " . open_quote ($template));

The space in that string literal looks like you’re trying to avoid some of the 
problems with the 1-argument form of IO::File-new().  Unless $template can be a 
dash to open STDIN, there is no good reason to use the 1-argument form of 
IO::File->new().

perlcritic will complain about the 2-argument form of open(), which the 
1-argument form of IO::File->new() calls, even on the gentlest perlcritic 
setting:

  http://goo.gl/xoQq3r

I realize that the previous version of the code also used the 1-argument form, 
but it looks like the Autom4te::XFile implementation that comes with Autoconf 
2.69 was changed to allow use of the 2-argument form:

  https://lists.gnu.org/archive/html/automake-patches/2012-03/msg00111.html

> my ($sym) = /^\#\s*\w+\s+(\w+)/

One of the things perlcritic --harsh will insist on are ‘x’ modifiers on 
regexes, and this RE is a good reason why.  It’s complex enough to benefit from 
comments:

    my ($sym) = m{
        ^\#         # a comment starting at the beginning of the line
        \s*\w+      # followed by optional whitespace and a word
        \s+         # followed by mandatory whitespace
        (\w+)       # and the symbol we actually care about
    }x or next;

I’m guessing that this is matching a comment of some kind, and “word” on the 
second RE line should be replaced with a more specific term.  Again, I don’t 
really know what this code is trying to accomplish.