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
