do {} while (0) nitpick

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

do {} while (0) nitpick

John Naylor-2
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

do-while-zero.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

Tom Lane-2
John Naylor <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

Bruce Momjian
On Thu, Apr 30, 2020 at 09:51:10PM -0400, Tom Lane wrote:

> John Naylor <[hidden email]> 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  <[hidden email]>        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 +


Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

David Steele
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 <[hidden email]> 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
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

Tom Lane-2
David Steele <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

Oleksandr Shulgin
In reply to this post by Bruce Momjian
On Fri, May 1, 2020 at 3:52 AM Bruce Momjian <[hidden email]> wrote:

>
> On Thu, Apr 30, 2020 at 09:51:10PM -0400, Tom Lane wrote:
> > John Naylor <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

Jesse Zhang
In reply to this post by Tom Lane-2
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


Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

Tom Lane-2
Jesse Zhang <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

Andrew Dunstan-8
In reply to this post by Tom Lane-2

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



Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

Andrew Dunstan-8

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



Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

Tom Lane-2
Andrew Dunstan <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

Andrew Dunstan-8

On 5/6/20 3:24 PM, Tom Lane wrote:

> Andrew Dunstan <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

David Steele
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
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: do {} while (0) nitpick

Andrew Dunstan-8

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