Re: perl checking

These two small patches allow us to run "perl -cw" cleanly on all our
perl code.

One patch silences a warning from convutils.pl about the unportability
of the literal 0x100000000. We've run for many years without this giving
us a problem, so I think we can turn the warning off pretty safely.

The other patch provides a dummy library that emulates just enough of
the Win32 perl infrastructure to allow us to run these checks. That
means that Unix-based developers who might want to make changes in the
msvc code can actually run a check against their code without having to
put it on a Windows machine. The invocation goes like this (to check
Mkvcbuild.pl for example):

PERL5LIB=src/tools/msvc/dummylib perl -cw src/tools/Mkvcbuild.pm

This also allows us to check src/tools/win32tzlist.pl.

In due course I'll submit a script to automate this syntax checking.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 5/18/18 14:02, Andrew Dunstan wrote:
> These two small patches allow us to run "perl -cw" cleanly on all our
> perl code.

It's not clear to me what that really means. My understanding is that
perl "warnings" are primarily a run-time instrument, unlike 'use strict'
and perl -c. I have been playing with a private branch that adds 'use
warnings' next to 'use strict' across the perl scripts, and there are a
number of warnings that pop up at run time. The fact that you get even
more warnings at compile time makes me wonder.

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

On 05/18/2018 09:05 PM, Peter Eisentraut wrote:
> On 5/18/18 14:02, Andrew Dunstan wrote:
>> These two small patches allow us to run "perl -cw" cleanly on all our
>> perl code.
> It's not clear to me what that really means. My understanding is that
> perl "warnings" are primarily a run-time instrument, unlike 'use strict'
> and perl -c. I have been playing with a private branch that adds 'use
> warnings' next to 'use strict' across the perl scripts, and there are a
> number of warnings that pop up at run time. The fact that you get even
> more warnings at compile time makes me wonder.
>

Mike Blackwell is working on some things that will help us lower the
severity of our perlcritic checks. One of those things will almost
certainly be to add "use warnings;" in quite a few places, so let's make
sure we don't duplicate effort.

Essentially "perl -cw" will make dure it can comoile the file and then
print warnings about those things it can detect at compile time. I have
found it a useful tool.

More importantly, there are several files in our Windows suite that a
Unix-based developer can't check even for compilation success, let alone
warnings, because they refer to libraries that only exist on Windows.
That's what the tiny dummy library is designed to fix.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote in <5a6d6de8-cff8-1ffb-946c-ccf381800ea1(at)2ndQuadrant(dot)com>
>
> These two small patches allow us to run "perl -cw" cleanly on all our
> perl code.
>
>
> One patch silences a warning from convutils.pl about the unportability
> of the literal 0x100000000. We've run for many years without this
> giving us a problem, so I think we can turn the warning off pretty
> safely.

It was introduced by aeed17d000 (in 2017). The history of the
file is rather short. Over 32-bit values do not apperar as a
character so there's no problem in ignoring the warning for now,
but can't we use bigint to silence it instead?

> The other patch provides a dummy library that emulates just enough of
> the Win32 perl infrastructure to allow us to run these checks. That
> means that Unix-based developers who might want to make changes in the
> msvc code can actually run a check against their code without having
> to put it on a Windows machine. The invocation goes like this (to
> check Mkvcbuild.pl for example):
>
>
> PERL5LIB=src/tools/msvc/dummylib perl -cw src/tools/Mkvcbuild.pm
>
>
> This also allows us to check src/tools/win32tzlist.pl.
>
>
> In due course I'll submit a script to automate this syntax checking.
>
>
> cheers
>
>
> andrew

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 05/22/2018 04:11 AM, Kyotaro HORIGUCHI wrote:
> At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote in <5a6d6de8-cff8-1ffb-946c-ccf381800ea1(at)2ndQuadrant(dot)com>
>> These two small patches allow us to run "perl -cw" cleanly on all our
>> perl code.
>>
>>
>> One patch silences a warning from convutils.pl about the unportability
>> of the literal 0x100000000. We've run for many years without this
>> giving us a problem, so I think we can turn the warning off pretty
>> safely.
> It was introduced by aeed17d000 (in 2017). The history of the
> file is rather short. Over 32-bit values do not apperar as a
> character so there's no problem in ignoring the warning for now,
> but can't we use bigint to silence it instead?
>

It would impose an additional dependency. bigint isn't installed by
default on many systems AFAICT, so I think we'd need a better reason
than this to require it.

I was a little optimistic about claiming that 'perl -cw' would run
cleanly with these two patches - there's a little remediation that will
be required in the src/msvc/tools directory. These patches at least let
it run to completion.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> writes:
> On 05/22/2018 04:11 AM, Kyotaro HORIGUCHI wrote:
>> At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote in <5a6d6de8-cff8-1ffb-946c-ccf381800ea1(at)2ndQuadrant(dot)com>
>>> One patch silences a warning from convutils.pl about the unportability
>>> of the literal 0x100000000. We've run for many years without this
>>> giving us a problem, so I think we can turn the warning off pretty
>>> safely.

>> It was introduced by aeed17d000 (in 2017). The history of the
>> file is rather short. Over 32-bit values do not apperar as a
>> character so there's no problem in ignoring the warning for now,
>> but can't we use bigint to silence it instead?

> It would impose an additional dependency. bigint isn't installed by
> default on many systems AFAICT, so I think we'd need a better reason
> than this to require it.

I agree with not adding a dependency (although FWIW, bigint does seem
to be there in my minimal perl setups). But can't we fix it like this:

- elsif ($in < 0x100000000)
+ elsif ($in <= 0xffffffff)

At least in a quick test here, "-cw" doesn't moan about 0xffffffff.

For consistency, the other arms of the "if" should be adjusted
similarly.

regards, tom lane

On 05/22/2018 10:09 AM, Tom Lane wrote:
> Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> writes:
>> On 05/22/2018 04:11 AM, Kyotaro HORIGUCHI wrote:
>>> At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote in <5a6d6de8-cff8-1ffb-946c-ccf381800ea1(at)2ndQuadrant(dot)com>
>>>> One patch silences a warning from convutils.pl about the unportability
>>>> of the literal 0x100000000. We've run for many years without this
>>>> giving us a problem, so I think we can turn the warning off pretty
>>>> safely.
>>> It was introduced by aeed17d000 (in 2017). The history of the
>>> file is rather short. Over 32-bit values do not apperar as a
>>> character so there's no problem in ignoring the warning for now,
>>> but can't we use bigint to silence it instead?
>> It would impose an additional dependency. bigint isn't installed by
>> default on many systems AFAICT, so I think we'd need a better reason
>> than this to require it.
> I agree with not adding a dependency (although FWIW, bigint does seem
> to be there in my minimal perl setups). But can't we fix it like this:
>
> - elsif ($in < 0x100000000)
> + elsif ($in <= 0xffffffff)
>
> At least in a quick test here, "-cw" doesn't moan about 0xffffffff.
>
> For consistency, the other arms of the "if" should be adjusted
> similarly.
>
>

Yeah. I tested this on the oldest 32 but perls I could find, the msys
and activestate perls on the XP machine that runs frogmouth and friends.
Even though they both have an ivsize of 4, the arithmetic seems to work
properly. Perhaps they store larger numbers as doubles, which you should
be able to do exactly up to about 52 bit integers. The other 32 bit
machine I have is an Ubuntu 16.04 VM, but there the perl has an ivsize
of 8, so of course it does the right thing.

We don't normally use these scripts anyway, so I'll go with this
suggestion without further investigation.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

At Tue, 22 May 2018 15:02:46 -0400, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote in <a70c49ec-d816-9fd6-1565-38fb20cc7206(at)2ndQuadrant(dot)com>
> > - elsif ($in < 0x100000000)
> > + elsif ($in <= 0xffffffff)

This is one of my thougts and the reason for regarding it sour is
the following.

> > For consistency, the other arms of the "if" should be adjusted
> > similarly.

> Yeah. I tested this on the oldest 32 but perls I could find, the msys
> and activestate perls on the XP machine that runs frogmouth and
> friends. Even though they both have an ivsize of 4, the arithmetic
> seems to work properly. Perhaps they store larger numbers as doubles,
> which you should be able to do exactly up to about 52 bit
> integers. The other 32 bit machine I have is an Ubuntu 16.04 VM, but
> there the perl has an ivsize of 8, so of course it does the right
> thing.
>
> We don't normally use these scripts anyway, so I'll go with this
> suggestion without further investigation.

Agreed. I'm fine with the direction.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 05/18/2018 02:02 PM, Andrew Dunstan wrote:
>
> These two small patches allow us to run "perl -cw" cleanly on all our
> perl code.
>
>
> One patch silences a warning from convutils.pl about the unportability
> of the literal 0x100000000. We've run for many years without this
> giving us a problem, so I think we can turn the warning off pretty
> safely.
>
>
> The other patch provides a dummy library that emulates just enough of
> the Win32 perl infrastructure to allow us to run these checks. That
> means that Unix-based developers who might want to make changes in the
> msvc code can actually run a check against their code without having
> to put it on a Windows machine. The invocation goes like this (to
> check Mkvcbuild.pl for example):
>
>
>    PERL5LIB=src/tools/msvc/dummylib perl -cw src/tools/Mkvcbuild.pm
>
>
> This also allows us to check src/tools/win32tzlist.pl.
>
>
> In due course I'll submit a script to automate this syntax checking.
>
>
>

Here is the latest version of the second patch, this time with warnings
about redefinition of some subroutines suppressed. These mostly occur
because in a few cases we have multiple packages in a single file.

This allows the following command to pass cleanly:

{ find . -type f -a -name '*.p[lm]' -print; find . -type f -perm
-100 -exec file {} \; -print | egrep -i ':.*perl[0-9]*\>' |cut -d:
-f1 ; } | sort -u |
PERL5LIB=src/test/perl:src/test/ssl:src/bin/pg_rewind:src/backend/catalog:src/backend/utils/mb/Unicode:src/tools/msvc/dummylib:src/tools/msvc
xargs -L 1 perl -cw

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sun, May 27, 2018 at 11:42 AM, Andrew Dunstan
<andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
>
>
> On 05/18/2018 02:02 PM, Andrew Dunstan wrote:
>>
>>
>> These two small patches allow us to run "perl -cw" cleanly on all our perl
>> code.
>>
>>
>> One patch silences a warning from convutils.pl about the unportability of
>> the literal 0x100000000. We've run for many years without this giving us a
>> problem, so I think we can turn the warning off pretty safely.
>>
>>
>> The other patch provides a dummy library that emulates just enough of the
>> Win32 perl infrastructure to allow us to run these checks. That means that
>> Unix-based developers who might want to make changes in the msvc code can
>> actually run a check against their code without having to put it on a
>> Windows machine. The invocation goes like this (to check Mkvcbuild.pl for
>> example):
>>
>>
>> PERL5LIB=src/tools/msvc/dummylib perl -cw src/tools/Mkvcbuild.pm
>>
>>
>> This also allows us to check src/tools/win32tzlist.pl.
>>
>>
>> In due course I'll submit a script to automate this syntax checking.
>>
>>
>>
>
>
> Here is the latest version of the second patch, this time with warnings
> about redefinition of some subroutines suppressed. These mostly occur
> because in a few cases we have multiple packages in a single file.
>
> This allows the following command to pass cleanly:
>
> { find . -type f -a -name '*.p[lm]' -print; find . -type f -perm
> -100 -exec file {} \; -print | egrep -i ':.*perl[0-9]*\>' |cut -d:
> -f1 ; } | sort -u |
>
> PERL5LIB=src/test/perl:src/test/ssl:src/bin/pg_rewind:src/backend/catalog:src/backend/utils/mb/Unicode:src/tools/msvc/dummylib:src/tools/msvc
> xargs -L 1 perl -cw
>
>

Attached version actually does what's advertised above.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services