[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] autoheader: check templates of all config headers
From: |
Daniel Elstner |
Subject: |
Re: [PATCH] autoheader: check templates of all config headers |
Date: |
Wed, 21 Dec 2016 17:15:46 +0100 |
Hi,
sorry for taking so long to get back to this. But I see a release is
imminent, so I figured better late than never.
I've updated my patch and rebased it onto current master. I would be
cool if it could make the release.
On Tue, 2015-12-15 at 18:13 -0700, Warren Young wrote:
> 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:
OK, I addressed some of these issues in my updated and rebased patch.
> > sub templates_for_header ($)
> Don’t use subroutine prototypes.
Got rid of it.
> > 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.
Done.
> > 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().
Agreed, and it turns out that it has already been changed to the two-
argument form in current master. I followed suit in my updated patch.
> > 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.
Left it as is for now as I just took it over from the original code.
Cheers,
--Daniel
0001-autoheader-check-templates-of-all-config-headers.patch
Description: Text Data
- Re: [PATCH] autoheader: check templates of all config headers,
Daniel Elstner <=