Horton

Thread

Commits

  1. Get rid of trailing semicolons in C macro definitions.

  1. do {} while (0) nitpick

    John Naylor <john.naylor@2ndquadrant.com> — 2020-05-01T01:08:02Z

    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

  2. Re: do {} while (0) nitpick

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-01T01:51:10Z

    John Naylor <john.naylor@2ndquadrant.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

  3. Re: do {} while (0) nitpick

    Bruce Momjian <bruce@momjian.us> — 2020-05-01T01:52:36Z

    On Thu, Apr 30, 2020 at 09:51:10PM -0400, Tom Lane wrote:
> John Naylor <john.naylor@2ndquadrant.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@momjian.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 +

  4. Re: do {} while (0) nitpick

    David Steele <david@pgmasters.net> — 2020-05-01T13:26:40Z

    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.naylor@2ndquadrant.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@pgmasters.net

  5. Re: do {} while (0) nitpick

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-01T21:32:11Z

    David Steele <david@pgmasters.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

  6. Re: do {} while (0) nitpick

    Oleksandr Shulgin <oleksandr.shulgin@zalando.de> — 2020-05-04T08:38:03Z

    On Fri, May 1, 2020 at 3:52 AM Bruce Momjian <bruce@momjian.us> wrote:
>
> On Thu, Apr 30, 2020 at 09:51:10PM -0400, Tom Lane wrote:
> > John Naylor <john.naylor@2ndquadrant.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

  7. Re: do {} while (0) nitpick

    Jesse Zhang <sbjesse@gmail.com> — 2020-05-04T15:01:56Z

    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

  8. Re: do {} while (0) nitpick

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-04T15:28:37Z

    Jesse Zhang <sbjesse@gmail.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

  9. Re: do {} while (0) nitpick

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-05-04T22:44:14Z

    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

  10. Re: do {} while (0) nitpick

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-05-06T18:06:41Z

    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

  11. Re: do {} while (0) nitpick

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-06T19:24:40Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.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

  12. Re: do {} while (0) nitpick

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-05-06T22:28:46Z

    On 5/6/20 3:24 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.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

  13. Re: do {} while (0) nitpick

    David Steele <david@pgmasters.net> — 2020-05-06T22:39:01Z

    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@pgmasters.net

  14. Re: do {} while (0) nitpick

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-05-07T12:24:38Z

    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