Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: cleaning perl code
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: <5b2ae2b0-8b09-d153-275a-24e7b2ac7989@2ndQuadrant.com>
Date: Sat, 11 Apr 2020 10:27:58 -0700
Cc: Noah Misch <noah@leadboat.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <4FFD55FA-0728-4DC3-AF93-0246FE555C10@enterprisedb.com>
References: <e56f6fb6-38d5-8450-577c-24eb5d63f2e1@2ndQuadrant.com>
 <20200411043014.GA590991@rfd.leadboat.com>
 <0a4406d5-b428-4831-8ed9-c36764360f57@2ndQuadrant.com>
 <97813D8D-F185-4998-8CF8-5678746E7DEE@enterprisedb.com>
 <5b2ae2b0-8b09-d153-275a-24e7b2ac7989@2ndQuadrant.com>
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Precedence: bulk


> On Apr 11, 2020, at 9:47 AM, Andrew Dunstan =
<andrew.dunstan@2ndquadrant.com> wrote:
>=20
>=20
> On 4/11/20 12:28 PM, Mark Dilger wrote:
>>=20
>>> On Apr 11, 2020, at 9:13 AM, Andrew Dunstan =
<andrew.dunstan@2ndquadrant.com> wrote:
>> Hi Andrew.  I appreciate your interest and efforts here.  I hope you =
don't mind a few questions/observations about this effort:
>=20
>=20
> Not at all.
>=20
>=20
>>=20
>>> The
>>> last one fixes the mixture of high and low precedence boolean =
operators,
>> I did not spot examples of this in your diffs, but I assume you mean =
to prohibit conditionals like:
>>=20
>>    if ($a || $b and $c || $d)
>>=20
>> As I understand it, perl introduced low precedence operators =
precisely to allow this.  Why disallow it?
>=20
>=20
> The docs say:
>=20
>=20
>    Conway advises against combining the low-precedence booleans ( |and
>    or not| ) with the high-precedence boolean operators ( |&& || !| )
>    in the same expression. Unless you fully understand the differences
>    between the high and low-precedence operators, it is easy to
>    misinterpret expressions that use both. And even if you do
>    understand them, it is not always clear if the author actually
>    intended it.
>=20
>    |next| |if| |not ||$foo| ||| ||$bar||;  ||#not ok|
>    |next| |if| |!||$foo| ||| ||$bar||;     ||#ok|
>    |next| |if| |!( ||$foo| ||| ||$bar| |); ||#ok|

I don't think any of those three are ok, from a code review perspective, =
but it's not because high and low precedence operators were intermixed.

>>=20
>>> and the use of commas to separate statements
>> I don't understand the prejudice against commas used this way.  What =
is wrong with:
>>=20
>>    $i++, $j++ if defined $k;
>>=20
>> rather than:
>>=20
>>    if (defined $k)
>>    {
>>        $i++;
>>        $j++;
>>    }
>>=20
>=20
>=20
> I don't think the example is terribly clear. I have to look at it and
> think "Does it do $i++ if $k isn't defined?"

It works like the equivalent C-code:

    if (k)
        i++, j++;

which to my eyes is also fine.

I'm less concerned with which perlcritic features you enable than I am =
with accidentally submitting perl which looks fine to me but breaks the =
build.  I mostly use perl from within TAP tests, which I run locally =
before submission to the project.  Can your changes be integrated into =
the TAP_TESTS makefile target so that I get local errors about this =
stuff and can fix it before submitting a regression test to -hackers?

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company