Horton

Thread

Commits

  1. Avoid use of unportable hex constant in convutils.pm

  1. perl checking

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-05-18T18:02:39Z

    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

  2. Re: perl checking

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-05-19T01:05:41Z

    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

  3. Re: perl checking

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-05-19T02:58:21Z

    
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

  4. Re: perl checking

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-05-22T08:11:44Z

    At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote in <5a6d6de8-cff8-1ffb-946c-ccf381800ea1@2ndQuadrant.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

  5. Re: perl checking

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-05-22T12:49:54Z

    
On 05/22/2018 04:11 AM, Kyotaro HORIGUCHI wrote:
> At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote in <5a6d6de8-cff8-1ffb-946c-ccf381800ea1@2ndQuadrant.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

  6. Re: perl checking

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-05-22T14:09:02Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 05/22/2018 04:11 AM, Kyotaro HORIGUCHI wrote:
>> At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote in <5a6d6de8-cff8-1ffb-946c-ccf381800ea1@2ndQuadrant.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

  7. Re: perl checking

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-05-22T19:02:46Z

    
On 05/22/2018 10:09 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> On 05/22/2018 04:11 AM, Kyotaro HORIGUCHI wrote:
>>> At Fri, 18 May 2018 14:02:39 -0400, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote in <5a6d6de8-cff8-1ffb-946c-ccf381800ea1@2ndQuadrant.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

  8. Re: perl checking

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-05-23T03:39:57Z

    At Tue, 22 May 2018 15:02:46 -0400, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote in <a70c49ec-d816-9fd6-1565-38fb20cc7206@2ndQuadrant.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

  9. Re: perl checking

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-05-27T15:42:07Z

    
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

  10. Re: perl checking

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-05-28T21:23:34Z

    On Sun, May 27, 2018 at 11:42 AM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.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