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
Attachments:
do-while-zero.patchapplication/octet-stream; name=do-while-zero.patchDownload
diff --git a/contrib/btree_gist/btree_utils_num.h b/contrib/btree_gist/btree_utils_num.h
index 1fedfbe82d..cec6986172 100644
--- a/contrib/btree_gist/btree_utils_num.h
+++ b/contrib/btree_gist/btree_utils_num.h
@@ -74,7 +74,7 @@ typedef struct
(*(result)) += (float) ( ((double)(tmp)) / ( (double)(tmp) + ( ((double)(oupper))*0.49F - ((double)(olower))*0.49F ) ) ); \
(*(result)) *= (FLT_MAX / (((GISTENTRY *) PG_GETARG_POINTER(0))->rel->rd_att->natts + 1)); \
} \
-} while (0);
+} while (0)
/*
diff --git a/contrib/pg_trgm/trgm.h b/contrib/pg_trgm/trgm.h
index 0c34b96d80..b616953462 100644
--- a/contrib/pg_trgm/trgm.h
+++ b/contrib/pg_trgm/trgm.h
@@ -48,7 +48,7 @@ typedef char trgm[3];
*(((char*)(a))+0) = *(((char*)(b))+0); \
*(((char*)(a))+1) = *(((char*)(b))+1); \
*(((char*)(a))+2) = *(((char*)(b))+2); \
-} while(0);
+} while(0)
#ifdef KEEPONLYALNUM
#define ISWORDCHR(c) (t_isalpha(c) || t_isdigit(c))
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index e991385059..d09bc6d291 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -3859,7 +3859,7 @@ do { \
(path) = reparameterize_path_by_child(root, (path), child_rel); \
if ((path) == NULL) \
return NULL; \
-} while(0);
+} while(0)
#define REPARAMETERIZE_CHILD_PATH_LIST(pathlist) \
do { \
@@ -3870,7 +3870,7 @@ do { \
if ((pathlist) == NIL) \
return NULL; \
} \
-} while(0);
+} while(0)
Path *new_path;
ParamPathInfo *new_ppi;
diff --git a/src/backend/utils/sort/gen_qsort_tuple.pl b/src/backend/utils/sort/gen_qsort_tuple.pl
index 9ed6cfc7ea..eb0f7c5814 100644
--- a/src/backend/utils/sort/gen_qsort_tuple.pl
+++ b/src/backend/utils/sort/gen_qsort_tuple.pl
@@ -126,7 +126,7 @@ swapfunc(SortTuple *a, SortTuple *b, size_t n)
SortTuple t = *(a); \
*(a) = *(b); \
*(b) = t; \
- } while (0);
+ } while (0)
#define vecswap(a, b, n) if ((n) > 0) swapfunc(a, b, n)
diff --git a/src/include/access/hash.h b/src/include/access/hash.h
index 94b643cc77..7e7b1b73d8 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -148,7 +148,7 @@ typedef struct HashScanPosData
(scanpos).firstItem = 0; \
(scanpos).lastItem = 0; \
(scanpos).itemIndex = 0; \
- } while (0);
+ } while (0)
/*
* HashScanOpaqueData is private state for a hash index scan.
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 9a3acd26b7..f976ba0270 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -888,7 +888,7 @@ typedef BTScanPosData *BTScanPos;
(scanpos).buf = InvalidBuffer; \
(scanpos).lsn = InvalidXLogRecPtr; \
(scanpos).nextTupleOffset = 0; \
- } while (0);
+ } while (0)
/* We need one of these for each equality-type SK_SEARCHARRAY scan key */
typedef struct BTArrayKeyInfo
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
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 +
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
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
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
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@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
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.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
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
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
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