From: | Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> |
---|---|
To: | Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> |
Cc: | Noah Misch <noah(at)leadboat(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: cleaning perl code |
Date: | 2020-04-11 17:27:58 |
Message-ID: | 4FFD55FA-0728-4DC3-AF93-0246FE555C10@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> On Apr 11, 2020, at 9:47 AM, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
>
>
> On 4/11/20 12:28 PM, Mark Dilger wrote:
>>
>>> On Apr 11, 2020, at 9:13 AM, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
>> Hi Andrew. I appreciate your interest and efforts here. I hope you don't mind a few questions/observations about this effort:
>
>
> Not at all.
>
>
>>
>>> 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:
>>
>> if ($a || $b and $c || $d)
>>
>> As I understand it, perl introduced low precedence operators precisely to allow this. Why disallow it?
>
>
> The docs say:
>
>
> 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.
>
> |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.
>>
>>> and the use of commas to separate statements
>> I don't understand the prejudice against commas used this way. What is wrong with:
>>
>> $i++, $j++ if defined $k;
>>
>> rather than:
>>
>> if (defined $k)
>> {
>> $i++;
>> $j++;
>> }
>>
>
>
> 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?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2020-04-11 17:31:16 | Re: cleaning perl code |
Previous Message | Andrew Dunstan | 2020-04-11 17:01:54 | Re: cleaning perl code |