ExtUtils::MM_Any: Get rid of unused printf arguments in dir_target #84
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These printf arguments have been in flux since 0f2e87e, but although we had 6 at that point they were quickly chopped down to 4 in 2f9a3b6 and have remained at that number ever since. Change the code not to provide redundant arguments that it doesn't have to. Found with a nascent patch of mine to the core to detect redundant printf arguments: https://rt.perl.org/Public/Bug/Display.html?id=121025
bingos
added a commit
that referenced
this pull request
Jan 18, 2014
ExtUtils::MM_Any: Get rid of unused printf arguments in dir_target
p5p
pushed a commit
to Perl/perl5
that referenced
this pull request
Jun 21, 2014
Implement RT #121025 and add a "redundant" warning category that
currently only warns about redundant arguments to printf. Now similarly
to how we already warned about missing printf arguments:
$ ./miniperl -Ilib -we 'printf "%s\n", qw()'
Missing argument in printf at -e line 1.
We'll now warn about redundant printf arguments:
$ ./miniperl -Ilib -we 'printf "%s\n", qw(x y)'
Redundant argument in printf at -e line 1.
x
The motivation for this is that I recently fixed an insidious
long-standing 6 year old bug in a codebase I maintain that came down to
an issue that would have been detected by this warning.
Things to note about this patch:
* It found a some long-standing redundant printf arguments in our own
ExtUtils::MakeMaker code which I submitted fixes to in
Perl-Toolchain-Gang/ExtUtils-MakeMaker#84 and
Perl-Toolchain-Gang/ExtUtils-MakeMaker#86,
those fixes were merged into blead in v5.19.8-265-gb33b7ab
* This warning correctly handles format parameter indexes (e.g. "%1$s")
for some value of correctly. See the comment in t/op/sprintf2.t for
an extensive discussion of how I've handled that.
* We do the correct thing in my opinion when a pattern has redundant
arguments *and* an invalid printf format. E.g. for the pattern "%A%s"
with one argument we'll just warn about an invalid format as before,
but with two arguments we'll warn about the invalid format *and* the
redundant argument.
This helps to disambiguate cases where the user just meant to write a
literal "%SOMETHING" v.s. cases where he though "%S" might be a valid
printf format.
* I originally wrote this while the 5.19 series was under way, but Dave
Mitchell has noted that a warning like this should go into blead
after 5.20 is released:
"[...] I think it should go into blead just after 5.20 is
released, rather than now; I think it'd going to kick up a lot of
dust and we'll want to give CPAN module owners maximum lead time
to fix up their code. For example, if its generating warnings in
cpan/ code in blead, then we need those module authors to fix
their code, produce stable new releases, pull them back into
blead, and let them bed in before we start pushing out 5.20 RC
candidates"
I agree, but we could have our cake and eat it too if "use warnings"
didn't turn this on but an explicit "use warnings qw(redundant)" did.
Then in 5.22 we could make "use warnings" also import the "redundant"
category, and in the meantime you could turn this on
explicitly.
There isn't an existing feature for adding that kind of warning in
the core. And my attempts at doing so failed, see commentary in RT
#121025.
The warning needed to be added to a few places in sv.c because the "",
"%s" and "%-p" patterns all bypass the normal printf handling for
optimization purposes. The new warning works correctly on all of
them. See the tests in t/op/sprintf2.t.
It's worth mentioning that both Debian Clang 3.3-16 and GCC 4.8.2-12
warn about this in C code under -Wall:
$ cat redundant.c
#include <stdio.h>
int main(void) {
printf("%d\n", 123, 345);
return 0;
}
$ clang -Wall -o redundant redundant.c
redundant.c:4:25: warning: data argument not used by format string [-Wformat-extra-args]
printf("%d\n", 123, 345);
~~~~~~ ^
1 warning generated.
$ gcc -Wall -o redundant redundant.c
redundant.c: In function ‘main’:
redundant.c:4:5: warning: too many arguments for format [-Wformat-extra-args]
printf("%d\n", 123, 345);
^
So I'm not the first person to think that this might be generally
useful.
There are also other internal functions that could benefit from
missing/redundant warnings, e.g. pack. Neither of these things currently
warn, but should:
$ perl -wE 'say pack "AA", qw(x y z)'
xy
$ perl -wE 'say pack "AAAA", qw(x y z)'
xyz
I'll file a bug for that, and might take a stab at implementing it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These printf arguments have been in flux since 0f2e87e, but although we
had 6 at that point they were quickly chopped down to 4 in 2f9a3b6 and
have remained at that number ever since.
Change the code not to provide redundant arguments that it doesn't have
to. Found with a nascent patch of mine to the core to detect redundant
printf arguments: https://rt.perl.org/Public/Bug/Display.html?id=121025