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

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

 



On Oct 9, 2015, at 10:48 PM, Paul Eggert <eggert@xxxxxxxxxxx> 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.
_______________________________________________
Autoconf mailing list
Autoconf@xxxxxxx
https://lists.gnu.org/mailman/listinfo/autoconf




[Index of Archives]     [GCC Help]     [Kernel Discussion]     [RPM Discussion]     [Red Hat Development]     [Yosemite News]     [Linux USB]     [Samba]

  Powered by Linux