Re: do {} while (0) nitpick

Hi,

As I understand it, the point of having "do {} while (0)" in a
multi-statement macro is to turn it into a simple statement. As such,
ending with a semicolon in both the macro definition and the
invocation will turn it back into multiple statements, creating
confusion if someone were to invoke the macro in an "if" statement.
Even if that never happens, it seems good to keep them all consistent,
as in the attached patch.

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

John Naylor <john(dot)naylor(at)2ndquadrant(dot)com> writes:
> As I understand it, the point of having "do {} while (0)" in a
> multi-statement macro is to turn it into a simple statement.

Right.

> As such,
> ending with a semicolon in both the macro definition and the
> invocation will turn it back into multiple statements, creating
> confusion if someone were to invoke the macro in an "if" statement.

Yeah. I'd call these actual bugs, and perhaps even back-patch worthy.

regards, tom lane

On Thu, Apr 30, 2020 at 09:51:10PM -0400, Tom Lane wrote:
> John Naylor <john(dot)naylor(at)2ndquadrant(dot)com> writes:
> > As I understand it, the point of having "do {} while (0)" in a
> > multi-statement macro is to turn it into a simple statement.
>
> Right.
>
> > As such,
> > ending with a semicolon in both the macro definition and the
> > invocation will turn it back into multiple statements, creating
> > confusion if someone were to invoke the macro in an "if" statement.
>
> Yeah. I'd call these actual bugs, and perhaps even back-patch worthy.

Agreed. Those semicolons could easily create bugs.

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EnterpriseDB https://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On 4/30/20 9:52 PM, Bruce Momjian wrote:
> On Thu, Apr 30, 2020 at 09:51:10PM -0400, Tom Lane wrote:
>> John Naylor <john(dot)naylor(at)2ndquadrant(dot)com> writes:
>>> As I understand it, the point of having "do {} while (0)" in a
>>> multi-statement macro is to turn it into a simple statement.
>>
>> Right.
>>
>>> As such,
>>> ending with a semicolon in both the macro definition and the
>>> invocation will turn it back into multiple statements, creating
>>> confusion if someone were to invoke the macro in an "if" statement.
>>
>> Yeah. I'd call these actual bugs, and perhaps even back-patch worthy.
>
> Agreed. Those semicolons could easily create bugs.

+1. The patch looks good to me.

--
-David
david(at)pgmasters(dot)net

David Steele <david(at)pgmasters(dot)net> writes:
> On 4/30/20 9:52 PM, Bruce Momjian wrote:
>> On Thu, Apr 30, 2020 at 09:51:10PM -0400, Tom Lane wrote:
>>> Yeah. I'd call these actual bugs, and perhaps even back-patch worthy.

>> Agreed. Those semicolons could easily create bugs.

> +1. The patch looks good to me.

Grepping showed me that there were some not-do-while macros that
also had trailing semicolons. These seem just as broken, so I
fixed 'em all.

There are remaining instances of this antipattern in the flex-generated
scanners, which we can't do anything about; and in pl/plperl/ppport.h,
which we shouldn't do anything about because that's upstream-generated
code. (I wonder though if there's a newer version available.)

regards, tom lane

On Fri, May 1, 2020 at 3:52 AM Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> On Thu, Apr 30, 2020 at 09:51:10PM -0400, Tom Lane wrote:
> > John Naylor <john(dot)naylor(at)2ndquadrant(dot)com> writes:
> > > As I understand it, the point of having "do {} while (0)" in a
> > > multi-statement macro is to turn it into a simple statement.
> >
> > Right.
> >
> > > As such,
> > > ending with a semicolon in both the macro definition and the
> > > invocation will turn it back into multiple statements, creating
> > > confusion if someone were to invoke the macro in an "if" statement.
> >
> > Yeah. I'd call these actual bugs, and perhaps even back-patch worthy.
>
> Agreed. Those semicolons could easily create bugs.

It was a while ago that I last checked our Developer guide over at
PostgreSQL wiki website, but I wonder if this is a sort of issue that
modern linters would be able to recognize?

The only hit for "linting" search on the wiki is this page referring to the
developer meeting in Ottawa about a year ago:
https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting

> Other major projects include:
> ...
> Code linting

Anybody aware what's the current status of that effort?

Cheers,
--
Alex

Hi Tom,

On Fri, May 1, 2020 at 2:32 PM Tom Lane wrote:
>
> Grepping showed me that there were some not-do-while macros that
> also had trailing semicolons. These seem just as broken, so I
> fixed 'em all.
>

I'm curious: *How* are you able to discover those occurrences with grep?
I understand how John might have done it with his original patch: it's
quite clear the pattern he would look for looks like "while (0);" but
how did you find all these other macro definitions with a trailing
semicolon? My tiny brain can only imagine:

1. Either grep for trailing semicolon (OMG almost every line will come
up) and squint through the context the see the previous line has a
trailing backslash;
2. Or use some LLVM magic to spelunk through every macro definition and
look for a trailing semicolon

Cheers,
Jesse

Jesse Zhang <sbjesse(at)gmail(dot)com> writes:
> On Fri, May 1, 2020 at 2:32 PM Tom Lane wrote:
>> Grepping showed me that there were some not-do-while macros that
>> also had trailing semicolons. These seem just as broken, so I
>> fixed 'em all.

> I'm curious: *How* are you able to discover those occurrences with grep?

Um, well, actually, it was a little perl script with a state variable
to remember whether it was in a macro definition or not (set on seeing
a #define, unset when current line doesn't end with '\', complain if
set and line ends with ';').

regards, tom lane

On 5/1/20 5:32 PM, Tom Lane wrote:
>
> There are remaining instances of this antipattern in the flex-generated
> scanners, which we can't do anything about; and in pl/plperl/ppport.h,
> which we shouldn't do anything about because that's upstream-generated
> code. (I wonder though if there's a newer version available.)

I'll take a look. It's more than 10 years since we updated it.

cheers

andrew

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

On 5/4/20 6:44 PM, Andrew Dunstan wrote:
> On 5/1/20 5:32 PM, Tom Lane wrote:
>> There are remaining instances of this antipattern in the flex-generated
>> scanners, which we can't do anything about; and in pl/plperl/ppport.h,
>> which we shouldn't do anything about because that's upstream-generated
>> code. (I wonder though if there's a newer version available.)
>
> I'll take a look. It's more than 10 years since we updated it.
>
>

I tried this out with ppport.h from perl 5.30.2 which is what's on my
Fedora 31 workstation. It compiled fine, no warnings and the tests all
ran fine.

So we could update it. I'm just not sure there would be any great
benefit from doing so until we want to use some piece of perl API that
postdates 5.11.2, which is where our current file comes from.

I couldn't actually find an instance of the offending pattern in either
version of pport.h. What am I overlooking?

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:
> I tried this out with ppport.h from perl 5.30.2 which is what's on my
> Fedora 31 workstation. It compiled fine, no warnings and the tests all
> ran fine.
> So we could update it. I'm just not sure there would be any great
> benefit from doing so until we want to use some piece of perl API that
> postdates 5.11.2, which is where our current file comes from.

Yeah, perhaps not. Given our general desire not to break old toolchains,
it might be a long time before we want to require any new Perl APIs.

> I couldn't actually find an instance of the offending pattern in either
> version of pport.h. What am I overlooking?

My script was looking for any macro ending with ';', so it found these:

#define START_MY_CXT static my_cxt_t my_cxt;

# define XCPT_TRY_END JMPENV_POP;

# define XCPT_TRY_END Copy(oldTOP, top_env, 1, Sigjmp_buf);

Those don't seem like things we'd use directly, so it's mostly moot.

BTW, I looked around and could not find a package-provided ppport.h
at all on my Red Hat systems. What package is it in?

regards, tom lane

On 5/6/20 3:24 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> writes:
>> I tried this out with ppport.h from perl 5.30.2 which is what's on my
>> Fedora 31 workstation. It compiled fine, no warnings and the tests all
>> ran fine.
>> So we could update it. I'm just not sure there would be any great
>> benefit from doing so until we want to use some piece of perl API that
>> postdates 5.11.2, which is where our current file comes from.
> Yeah, perhaps not. Given our general desire not to break old toolchains,
> it might be a long time before we want to require any new Perl APIs.
>
>> I couldn't actually find an instance of the offending pattern in either
>> version of pport.h. What am I overlooking?
> My script was looking for any macro ending with ';', so it found these:
>
> #define START_MY_CXT static my_cxt_t my_cxt;
>
> # define XCPT_TRY_END JMPENV_POP;
>
> # define XCPT_TRY_END Copy(oldTOP, top_env, 1, Sigjmp_buf);
>
> Those don't seem like things we'd use directly, so it's mostly moot.

Yeah. My search was too specific.

The modern one has these too :-(

> BTW, I looked around and could not find a package-provided ppport.h
> at all on my Red Hat systems. What package is it in?

perl-Devel-PPPort contains a perl module that will write the file for
you like this:

perl -MDevel::PPPort -e 'Devel::PPPort::WriteFile();'

cheers

andrew

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

On 5/6/20 6:28 PM, Andrew Dunstan wrote:
> On 5/6/20 3:24 PM, Tom Lane wrote:
>
>> BTW, I looked around and could not find a package-provided ppport.h
>> at all on my Red Hat systems. What package is it in?
>
> perl-Devel-PPPort contains a perl module that will write the file for
> you like this:
>
> perl -MDevel::PPPort -e 'Devel::PPPort::WriteFile();'

FWIW, pgBackRest always shipped with the newest version of ppport.h we
were able to generate. This never caused any issues, but neither did we
have problems that forced us to upgrade.

The documentation seems to encourage this behavior:

Don't direct the users of your module to download Devel::PPPort . They
are most probably no XS writers. Also, don't make ppport.h optional.
Rather, just take the most recent copy of ppport.h that you can find
(e.g. by generating it with the latest Devel::PPPort release from CPAN),
copy it into your project, adjust your project to use it, and distribute
the header along with your module.

Regards,
--
-David
david(at)pgmasters(dot)net

On 5/6/20 6:39 PM, David Steele wrote:
> On 5/6/20 6:28 PM, Andrew Dunstan wrote:
>> On 5/6/20 3:24 PM, Tom Lane wrote:
>>
>>> BTW, I looked around and could not find a package-provided ppport.h
>>> at all on my Red Hat systems.  What package is it in?
>>
>> perl-Devel-PPPort contains a perl module that will write the file for
>> you like this:
>>
>>      perl -MDevel::PPPort -e 'Devel::PPPort::WriteFile();'
>
> FWIW, pgBackRest always shipped with the newest version of ppport.h we
> were able to generate. This never caused any issues, but neither did
> we have problems that forced us to upgrade.
>
> The documentation seems to encourage this behavior:
>
> Don't direct the users of your module to download Devel::PPPort . They
> are most probably no XS writers. Also, don't make ppport.h optional.
> Rather, just take the most recent copy of ppport.h that you can find
> (e.g. by generating it with the latest Devel::PPPort release from
> CPAN), copy it into your project, adjust your project to use it, and
> distribute the header along with your module.
>
>

I don't think we need to keep updating it, though. plperl is essentially
pretty stable.

cheers

andrew

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