use strict in all Perl programs

Started by Peter Eisentrautabout 9 years ago5 messages
#1Peter Eisentraut
peter.eisentraut@2ndquadrant.com
1 attachment(s)

Here is a patch to add 'use strict' to all Perl programs (that I could
find), or move it to the right place where it was already there. I
think that is a pretty standard thing to do nowadays.

I tried testing the changes in pgcheckdefines, but it just spits out
nonsense before and after.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

0001-Use-use-strict-in-all-Perl-programs.patchtext/x-patch; name=0001-Use-use-strict-in-all-Perl-programs.patchDownload
From 6db551f6ba2a9339051ecc7cabeb29ff59de2b26 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Sun, 4 Dec 2016 12:00:00 -0500
Subject: [PATCH 1/2] Use 'use strict' in all Perl programs

---
 contrib/seg/seg-validate.pl        | 35 +++++++++++-----------
 contrib/seg/sort-segments.pl       | 10 +++++--
 doc/src/sgml/mk_feature_tables.pl  |  2 ++
 src/pl/plperl/plc_perlboot.pl      |  2 ++
 src/test/locale/sort-test.pl       |  2 ++
 src/tools/msvc/build.pl            |  4 ++-
 src/tools/msvc/gendef.pl           |  6 ++--
 src/tools/msvc/pgflex.pl           |  6 ++--
 src/tools/pginclude/pgcheckdefines | 59 ++++++++++++++++++++++----------------
 src/tools/version_stamp.pl         | 18 ++++++++----
 10 files changed, 87 insertions(+), 57 deletions(-)

diff --git a/contrib/seg/seg-validate.pl b/contrib/seg/seg-validate.pl
index cb3fb9a099..b8957ed984 100755
--- a/contrib/seg/seg-validate.pl
+++ b/contrib/seg/seg-validate.pl
@@ -1,20 +1,23 @@
 #!/usr/bin/perl
-$integer = '[+-]?[0-9]+';
-$real    = '[+-]?[0-9]+\.[0-9]+';
-
-$RANGE     = '(\.\.)(\.)?';
-$PLUMIN    = q(\'\+\-\');
-$FLOAT     = "(($integer)|($real))([eE]($integer))?";
-$EXTENSION = '<|>|~';
-
-$boundary  = "($EXTENSION)?$FLOAT";
-$deviation = $FLOAT;
-
-$rule_1 = $boundary . $PLUMIN . $deviation;
-$rule_2 = $boundary . $RANGE . $boundary;
-$rule_3 = $boundary . $RANGE;
-$rule_4 = $RANGE . $boundary;
-$rule_5 = $boundary;
+
+use strict;
+
+my $integer = '[+-]?[0-9]+';
+my $real    = '[+-]?[0-9]+\.[0-9]+';
+
+my $RANGE     = '(\.\.)(\.)?';
+my $PLUMIN    = q(\'\+\-\');
+my $FLOAT     = "(($integer)|($real))([eE]($integer))?";
+my $EXTENSION = '<|>|~';
+
+my $boundary  = "($EXTENSION)?$FLOAT";
+my $deviation = $FLOAT;
+
+my $rule_1 = $boundary . $PLUMIN . $deviation;
+my $rule_2 = $boundary . $RANGE . $boundary;
+my $rule_3 = $boundary . $RANGE;
+my $rule_4 = $RANGE . $boundary;
+my $rule_5 = $boundary;
 
 
 print "$rule_5\n";
diff --git a/contrib/seg/sort-segments.pl b/contrib/seg/sort-segments.pl
index a465468d5b..04eafd92f2 100755
--- a/contrib/seg/sort-segments.pl
+++ b/contrib/seg/sort-segments.pl
@@ -2,6 +2,10 @@
 
 # this script will sort any table with the segment data type in its last column
 
+use strict;
+
+my @rows;
+
 while (<>)
 {
 	chomp;
@@ -10,11 +14,11 @@
 
 foreach (
 	sort {
-		@ar = split("\t", $a);
-		$valA = pop @ar;
+		my @ar = split("\t", $a);
+		my $valA = pop @ar;
 		$valA =~ s/[~<> ]+//g;
 		@ar = split("\t", $b);
-		$valB = pop @ar;
+		my $valB = pop @ar;
 		$valB =~ s/[~<> ]+//g;
 		$valA <=> $valB
 	} @rows)
diff --git a/doc/src/sgml/mk_feature_tables.pl b/doc/src/sgml/mk_feature_tables.pl
index 45dea798cd..93dab2132e 100644
--- a/doc/src/sgml/mk_feature_tables.pl
+++ b/doc/src/sgml/mk_feature_tables.pl
@@ -2,6 +2,8 @@
 
 # doc/src/sgml/mk_feature_tables.pl
 
+use strict;
+
 my $yesno = $ARGV[0];
 
 open PACK, $ARGV[1] or die;
diff --git a/src/pl/plperl/plc_perlboot.pl b/src/pl/plperl/plc_perlboot.pl
index d506d01163..bb2d009be0 100644
--- a/src/pl/plperl/plc_perlboot.pl
+++ b/src/pl/plperl/plc_perlboot.pl
@@ -1,5 +1,7 @@
 #  src/pl/plperl/plc_perlboot.pl
 
+use strict;
+
 use 5.008001;
 use vars qw(%_SHARED $_TD);
 
diff --git a/src/test/locale/sort-test.pl b/src/test/locale/sort-test.pl
index ce7b93c571..cb7e4934e4 100755
--- a/src/test/locale/sort-test.pl
+++ b/src/test/locale/sort-test.pl
@@ -1,4 +1,6 @@
 #! /usr/bin/perl
+
+use strict;
 use locale;
 
 open(INFILE, "<$ARGV[0]");
diff --git a/src/tools/msvc/build.pl b/src/tools/msvc/build.pl
index a5469cd289..2e7c54853a 100644
--- a/src/tools/msvc/build.pl
+++ b/src/tools/msvc/build.pl
@@ -2,6 +2,8 @@
 
 # src/tools/msvc/build.pl
 
+use strict;
+
 BEGIN
 {
 
@@ -68,6 +70,6 @@ BEGIN
 
 # report status
 
-$status = $? >> 8;
+my $status = $? >> 8;
 
 exit $status;
diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
index a6c43c2c39..3bcff7ffaf 100644
--- a/src/tools/msvc/gendef.pl
+++ b/src/tools/msvc/gendef.pl
@@ -1,11 +1,11 @@
-my @def;
-
-use warnings;
 use strict;
+use warnings;
 use 5.8.0;
 use File::Spec::Functions qw(splitpath catpath);
 use List::Util qw(max);
 
+my @def;
+
 #
 # Script that generates a .DEF file for all objects in a directory
 #
diff --git a/src/tools/msvc/pgflex.pl b/src/tools/msvc/pgflex.pl
index 474ce63e5c..3a42add0d2 100644
--- a/src/tools/msvc/pgflex.pl
+++ b/src/tools/msvc/pgflex.pl
@@ -2,12 +2,12 @@
 
 # src/tools/msvc/pgflex.pl
 
-# silence flex bleatings about file path style
-$ENV{CYGWIN} = 'nodosfilewarning';
-
 use strict;
 use File::Basename;
 
+# silence flex bleatings about file path style
+$ENV{CYGWIN} = 'nodosfilewarning';
+
 # assume we are in the postgres source root
 
 require 'src/tools/msvc/buildenv.pl' if -e 'src/tools/msvc/buildenv.pl';
diff --git a/src/tools/pginclude/pgcheckdefines b/src/tools/pginclude/pgcheckdefines
index 5db507070f..e166efa08d 100755
--- a/src/tools/pginclude/pgcheckdefines
+++ b/src/tools/pginclude/pgcheckdefines
@@ -20,14 +20,16 @@
 # src/tools/pginclude/pgcheckdefines
 #
 
+use strict;
+
 use Cwd;
 use File::Basename;
 
-$topdir = cwd();
+my $topdir = cwd();
 
 # Programs to use
-$FIND = "find";
-$MAKE = "make";
+my $FIND = "find";
+my $MAKE = "make";
 
 #
 # Build arrays of all the .c and .h files in the tree
@@ -38,6 +40,8 @@ $MAKE = "make";
 # Including these .h files would clutter the list of define'd symbols and
 # cause a lot of false-positive results.
 #
+my (@cfiles, @hfiles);
+
 open PIPE, "$FIND * -type f -name '*.c' |"
   or die "can't fork: $!";
 while (<PIPE>)
@@ -63,7 +67,9 @@ close PIPE or die "$FIND failed: $!";
 # a hash table.  To cover the possibility of multiple .h files defining
 # the same symbol, we make each hash entry a hash of filenames.
 #
-foreach $hfile (@hfiles)
+my %defines;
+
+foreach my $hfile (@hfiles)
 {
 	open HFILE, $hfile
 	  or die "can't open $hfile: $!";
@@ -82,9 +88,9 @@ foreach $hfile (@hfiles)
 # files it #include's.  Then extract all the symbols it tests for defined-ness,
 # and check each one against the previously built hashtable.
 #
-foreach $file (@hfiles, @cfiles)
+foreach my $file (@hfiles, @cfiles)
 {
-	($fname, $fpath) = fileparse($file);
+	my ($fname, $fpath) = fileparse($file);
 	chdir $fpath or die "can't chdir to $fpath: $!";
 
 	#
@@ -96,16 +102,18 @@ foreach $file (@hfiles, @cfiles)
 	# hence printing multiple definitions --- we keep the last one, which
 	# should come from the current Makefile.
 	#
+	my $MAKECMD;
+
 	if (-f "Makefile" || -f "GNUmakefile")
 	{
 		$MAKECMD = "$MAKE -qp";
 	}
 	else
 	{
-		$subdir = $fpath;
+		my $subdir = $fpath;
 		chop $subdir;
-		$top_builddir = "..";
-		$tmp          = $fpath;
+		my $top_builddir = "..";
+		my $tmp          = $fpath;
 		while (($tmp = dirname($tmp)) ne '.')
 		{
 			$top_builddir = $top_builddir . "/..";
@@ -113,6 +121,9 @@ foreach $file (@hfiles, @cfiles)
 		$MAKECMD =
 "$MAKE -qp 'subdir=$subdir' 'top_builddir=$top_builddir' -f '$top_builddir/src/Makefile.global'";
 	}
+
+	my ($CPPFLAGS, $CFLAGS, $CFLAGS_SL, $PTHREAD_CFLAGS, $CC);
+
 	open PIPE, "$MAKECMD |"
 	  or die "can't fork: $!";
 	while (<PIPE>)
@@ -153,15 +164,15 @@ foreach $file (@hfiles, @cfiles)
 	# "gcc -H" reports inclusions on stderr as "... filename" where the
 	# number of dots varies according to nesting depth.
 	#
-	@includes = ();
-	$COMPILE  = "$CC $CPPFLAGS $CFLAGS -H -E $fname";
+	my @includes = ();
+	my $COMPILE  = "$CC $CPPFLAGS $CFLAGS -H -E $fname";
 	open PIPE, "$COMPILE 2>&1 >/dev/null |"
 	  or die "can't fork: $!";
 	while (<PIPE>)
 	{
 		if (m/^\.+ (.*)/)
 		{
-			$include = $1;
+			my $include = $1;
 
 			# Ignore system headers (absolute paths); but complain if a
 			# .c file includes a system header before any PG header.
@@ -176,7 +187,7 @@ foreach $file (@hfiles, @cfiles)
 			$include =~ s|^\./||;
 
 			# Make path relative to top of tree
-			$ipath = $fpath;
+			my $ipath = $fpath;
 			while ($include =~ s|^\.\./||)
 			{
 				$ipath = dirname($ipath) . "/";
@@ -202,19 +213,17 @@ foreach $file (@hfiles, @cfiles)
 	#
 	open FILE, $fname
 	  or die "can't open $file: $!";
-	$inif = 0;
+	my $inif = 0;
 	while (<FILE>)
 	{
-		$line = $_;
+		my $line = $_;
 		if ($line =~ m/^\s*#\s*ifdef\s+(\w+)/)
 		{
-			$symbol = $1;
-			&checkit;
+			checkit($file, $1, @includes);
 		}
 		if ($line =~ m/^\s*#\s*ifndef\s+(\w+)/)
 		{
-			$symbol = $1;
-			&checkit;
+			checkit($file, $1, @includes);
 		}
 		if ($line =~ m/^\s*#\s*if\s+/)
 		{
@@ -224,8 +233,7 @@ foreach $file (@hfiles, @cfiles)
 		{
 			while ($line =~ s/\bdefined(\s+|\s*\(\s*)(\w+)//)
 			{
-				$symbol = $2;
-				&checkit;
+				checkit($file, $2, @includes);
 			}
 			if (!($line =~ m/\\$/))
 			{
@@ -243,6 +251,7 @@ exit 0;
 # Check an is-defined reference
 sub checkit
 {
+	my ($file, $symbol, @includes) = @_;
 
 	# Ignore if symbol isn't defined in any PG include files
 	if (!defined $defines{$symbol})
@@ -258,10 +267,10 @@ sub checkit
 	# occur after the use of the symbol.  Given our normal file layout,
 	# however, the risk is minimal.
 	#
-	foreach $deffile (keys %{ $defines{$symbol} })
+	foreach my $deffile (keys %{ $defines{$symbol} })
 	{
 		return if $deffile eq $file;
-		foreach $reffile (@includes)
+		foreach my $reffile (@includes)
 		{
 			return if $deffile eq $reffile;
 		}
@@ -273,7 +282,7 @@ sub checkit
 	#
 	if ($file =~ m/\.h$/)
 	{
-		foreach $deffile (keys %{ $defines{$symbol} })
+		foreach my $deffile (keys %{ $defines{$symbol} })
 		{
 			return if $deffile eq 'src/include/c.h';
 			return if $deffile eq 'src/include/postgres.h';
@@ -284,7 +293,7 @@ sub checkit
 	}
 
 	#
-	@places = keys %{ $defines{$symbol} };
+	my @places = keys %{ $defines{$symbol} };
 	print "$file references $symbol, defined in @places\n";
 
 	# print "includes: @includes\n";
diff --git a/src/tools/version_stamp.pl b/src/tools/version_stamp.pl
index 3edd7bedaf..fea10344a2 100755
--- a/src/tools/version_stamp.pl
+++ b/src/tools/version_stamp.pl
@@ -20,14 +20,18 @@
 # "devel", "alphaN", "betaN", "rcN".
 #
 
+use strict;
+
 # Major version is hard-wired into the script.  We update it when we branch
 # a new development version.
-$majorversion = 10;
+my $majorversion = 10;
 
 # Validate argument and compute derived variables
-$minor = shift;
+my $minor = shift;
 defined($minor) || die "$0: missing required argument: minor-version\n";
 
+my ($dotneeded, $numericminor);
+
 if ($minor =~ m/^\d+$/)
 {
 	$dotneeded    = 1;
@@ -58,6 +62,8 @@
 	die "$0: minor-version must be N, devel, alphaN, betaN, or rcN\n";
 }
 
+my $fullversion;
+
 # Create various required forms of the version number
 if ($dotneeded)
 {
@@ -67,13 +73,13 @@
 {
 	$fullversion = $majorversion . $minor;
 }
-$numericversion = $majorversion . "." . $numericminor;
-$padnumericversion = sprintf("%d%04d", $majorversion, $numericminor);
+my $numericversion = $majorversion . "." . $numericminor;
+my $padnumericversion = sprintf("%d%04d", $majorversion, $numericminor);
 
 # Get the autoconf version number for eventual nag message
 # (this also ensures we're in the right directory)
 
-$aconfver = "";
+my $aconfver = "";
 open(FILE, "configure.in") || die "could not read configure.in: $!\n";
 while (<FILE>)
 {
@@ -90,7 +96,7 @@
 
 # Update configure.in and other files that contain version numbers
 
-$fixedfiles = "";
+my $fixedfiles = "";
 
 sed_file("configure.in",
 "-e 's/AC_INIT(\\[PostgreSQL\\], \\[[0-9a-z.]*\\]/AC_INIT([PostgreSQL], [$fullversion]/'"
-- 
2.11.0

#2Michael Paquier
michael.paquier@gmail.com
In reply to: Peter Eisentraut (#1)
Re: use strict in all Perl programs

On Sat, Dec 31, 2016 at 3:07 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Here is a patch to add 'use strict' to all Perl programs (that I could
find), or move it to the right place where it was already there. I
think that is a pretty standard thing to do nowadays.

I tried testing the changes in pgcheckdefines, but it just spits out
nonsense before and after.

What about adding as well "use warnings"? That's standard in all the TAP tests.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Michael Paquier (#2)
Re: use strict in all Perl programs

On 12/31/16 1:34 AM, Michael Paquier wrote:

On Sat, Dec 31, 2016 at 3:07 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Here is a patch to add 'use strict' to all Perl programs (that I could
find), or move it to the right place where it was already there. I
think that is a pretty standard thing to do nowadays.

committed that

What about adding as well "use warnings"? That's standard in all the TAP tests.

'use strict' can be statically checked using perl -c, but 'use warnings'
is run-time behavior, so one would have to extensively test the involved
programs. Some cursory checking already reveals that this is going to
need to more investigation. So in principle yes, but maybe later.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4David Steele
david@pgmasters.net
In reply to: Peter Eisentraut (#3)
Re: use strict in all Perl programs

On 1/5/17 12:37 PM, Peter Eisentraut wrote:

On 12/31/16 1:34 AM, Michael Paquier wrote:

On Sat, Dec 31, 2016 at 3:07 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Here is a patch to add 'use strict' to all Perl programs (that I could
find), or move it to the right place where it was already there. I
think that is a pretty standard thing to do nowadays.

committed that

What about adding as well "use warnings"? That's standard in all the TAP tests.

'use strict' can be statically checked using perl -c, but 'use warnings'
is run-time behavior, so one would have to extensively test the involved
programs. Some cursory checking already reveals that this is going to
need to more investigation. So in principle yes, but maybe later.

With regard to warnings, I prefer to use:

use warnings FATAL => qw(all);

This transforms all warnings into errors rather than just printing a
message to stderr, which is very easy to miss among the other output.

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Michael Paquier
michael.paquier@gmail.com
In reply to: David Steele (#4)
Re: use strict in all Perl programs

On Fri, Jan 6, 2017 at 11:13 PM, David Steele <david@pgmasters.net> wrote:

With regard to warnings, I prefer to use:

use warnings FATAL => qw(all);

This transforms all warnings into errors rather than just printing a message
to stderr, which is very easy to miss among the other output.

Interesting. A couple of warnings have slipped a couple of times in
some TAP tests like those of pg_rewind, so it could be useful to
switch to that at least for the tests by detault.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers