Re: do {} while (0) nitpick
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 Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: do {} while (0) nitpick
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 da...@pgmasters.net
Re: do {} while (0) nitpick
On 5/6/20 3:24 PM, Tom Lane wrote: > Andrew Dunstan 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 Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: do {} while (0) nitpick
Andrew Dunstan 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_CXTstatic 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
Re: do {} while (0) nitpick
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 Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: do {} while (0) nitpick
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 Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: do {} while (0) nitpick
Jesse Zhang 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
Re: do {} while (0) nitpick
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
Re: do {} while (0) nitpick
On Fri, May 1, 2020 at 3:52 AM Bruce Momjian wrote:
>
> On Thu, Apr 30, 2020 at 09:51:10PM -0400, Tom Lane wrote:
> > John Naylor 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
Re: do {} while (0) nitpick
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 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
da...@pgmasters.net
Re: do {} while (0) nitpick
On Thu, Apr 30, 2020 at 09:51:10PM -0400, Tom Lane wrote:
> John Naylor 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 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 +
Re: do {} while (0) nitpick
John Naylor 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
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 Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
do-while-zero.patch
Description: Binary data
