Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On Tue, Mar 10, 2020 at 12:22:32AM +, Smith, Peter wrote: > Sorry I was AWOL for a couple of months. Thanks for taking the patch > further, and committing it. No problem, I am glad to see you around. -- Michael signature.asc Description: PGP signature
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
Hi Michael, Sorry I was AWOL for a couple of months. Thanks for taking the patch further, and committing it. Kind Regards --- Peter Smith Fujitsu Australia
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On Fri, Jan 31, 2020 at 02:46:16PM +0900, Michael Paquier wrote: > Actually, thinking more about it, I'd rather remove this part as well, > keeping only the change in the third paragraph of this comment block. I have tweaked a bit the comments in this area, and applied the patch. I'll begin a new thread with the rest of the refactoring. There are a couple of things I'd like to double-check first. -- Michael signature.asc Description: PGP signature
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On Fri, Jan 31, 2020 at 02:15:42PM +0900, Kyotaro Horiguchi wrote: > As a cross check, it cleanly applied and worked as expected. The > fallback definition of StaticAssertDecl for C worked for gcc 8.3. Thanks for the review. > - * Macros to support compile-time assertion checks. > + * Macros to support compile-time and declaration assertion checks. > > All the StaticAssert things check compile-time assertion. I think > that the name StaticAssertDecl doesn't mean "declaration assertion", > but means "static assertion as a declaration". Is the change needed? Hmm. Yeah, that sounds right. > - * If the "condition" (a compile-time-constant expression) evaluates to > false, > - * throw a compile error using the "errmessage" (a string literal). > + * If the "condition" (a compile-time-constant or declaration expression) > + * evaluates to false, throw a compile error using the "errmessage" (a > + * string literal). > > I'm not sure what the "declaration expression" here means. I think > the term generally means "a variable declaration in expressions", > something like "r = y * (int x = blah)". In that sense, the parameter > for StaticAssertDecl is just a compile-time constant expression. Is it > a right rewrite? Actually, thinking more about it, I'd rather remove this part as well, keeping only the change in the third paragraph of this comment block. -- Michael signature.asc Description: PGP signature
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
At Fri, 31 Jan 2020 11:47:01 +0900, Michael Paquier wrote in > On Tue, Dec 24, 2019 at 02:47:29PM +0900, Michael Paquier wrote: > > I am still seeing that a couple of points need an extra lookup: > > - Addition of a Decl() in at least one header of the core code. > > I agree with the addition of Decl() definition in a header, and could > not think about something better than one for bufpage.h for the > all-zero check case, so I went with that. Attached is a 0001 which > adds the definition for StaticAssertDecl() for C and C++ for all code > paths. If there are no objections, I would like to commit this > version. There is no fancy refactoring in it, and small progress is > better than no progress. I have also reworked the comments in the > patch, and did some testing on Windows. As a cross check, it cleanly applied and worked as expected. The fallback definition of StaticAssertDecl for C worked for gcc 8.3. - * Macros to support compile-time assertion checks. + * Macros to support compile-time and declaration assertion checks. All the StaticAssert things check compile-time assertion. I think that the name StaticAssertDecl doesn't mean "declaration assertion", but means "static assertion as a declaration". Is the change needed? - * If the "condition" (a compile-time-constant expression) evaluates to false, - * throw a compile error using the "errmessage" (a string literal). + * If the "condition" (a compile-time-constant or declaration expression) + * evaluates to false, throw a compile error using the "errmessage" (a + * string literal). I'm not sure what the "declaration expression" here means. I think the term generally means "a variable declaration in expressions", something like "r = y * (int x = blah)". In that sense, the parameter for StaticAssertDecl is just a compile-time constant expression. Is it a right rewrite? > > - Perhaps unifying the fallback implementation between C and C++, with > > a closer lookup in particular at StaticAssertStmt() and StaticAssertExpr(). > > Seeing nothing happening on this side. I took a shot at all that, and > I have hacked my way through it with 0002 which is an attempt to unify > the fallback implementation for C and C++. This is not fully baked > yet, and it is perhaps a matter of taste if this makes the code more > readable or not. I think it does, because it reduces the parts > dedicated to assertion definitions from four to three. Anyway, let's > discuss about that. +1 as far as the unification is right. I'm not sure, but at least it worked for gcc 8.3. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On Tue, Dec 24, 2019 at 02:47:29PM +0900, Michael Paquier wrote:
> I am still seeing that a couple of points need an extra lookup:
> - Addition of a Decl() in at least one header of the core code.
I agree with the addition of Decl() definition in a header, and could
not think about something better than one for bufpage.h for the
all-zero check case, so I went with that. Attached is a 0001 which
adds the definition for StaticAssertDecl() for C and C++ for all code
paths. If there are no objections, I would like to commit this
version. There is no fancy refactoring in it, and small progress is
better than no progress. I have also reworked the comments in the
patch, and did some testing on Windows.
> - Perhaps unifying the fallback implementation between C and C++, with
> a closer lookup in particular at StaticAssertStmt() and StaticAssertExpr().
Seeing nothing happening on this side. I took a shot at all that, and
I have hacked my way through it with 0002 which is an attempt to unify
the fallback implementation for C and C++. This is not fully baked
yet, and it is perhaps a matter of taste if this makes the code more
readable or not. I think it does, because it reduces the parts
dedicated to assertion definitions from four to three. Anyway, let's
discuss about that.
--
Michael
From 3872af56ad51ba2f6d927ffd6e7f79a0bd08551a Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 31 Jan 2020 10:34:30 +0900
Subject: [PATCH 1/2] Add declaration-level assertions
---
src/include/c.h| 24 --
src/include/storage/bufpage.h | 10
src/backend/storage/page/bufpage.c | 9 +--
src/backend/utils/adt/lockfuncs.c | 6 +
src/backend/utils/misc/guc.c | 39 ++
src/common/relpath.c | 3 +++
src/bin/pg_dump/pg_dump_sort.c | 3 +++
7 files changed, 79 insertions(+), 15 deletions(-)
diff --git a/src/include/c.h b/src/include/c.h
index 6898229b43..b2966f9edf 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -826,14 +826,16 @@ extern void ExceptionalCondition(const char *conditionName,
#endif
/*
- * Macros to support compile-time assertion checks.
+ * Macros to support compile-time and declaration assertion checks.
*
- * If the "condition" (a compile-time-constant expression) evaluates to false,
- * throw a compile error using the "errmessage" (a string literal).
+ * If the "condition" (a compile-time-constant or declaration expression)
+ * evaluates to false, throw a compile error using the "errmessage" (a
+ * string literal).
*
* gcc 4.6 and up supports _Static_assert(), but there are bizarre syntactic
- * placement restrictions. These macros make it safe to use as a statement
- * or in an expression, respectively.
+ * placement restrictions. Compile-time macros make it safe to use as a
+ * statement or in an expression, respectively. Declaration macros are
+ * useful at file scope.
*
* Otherwise we fall back on a kluge that assumes the compiler will complain
* about a negative width for a struct bit-field. This will not include a
@@ -845,11 +847,15 @@ extern void ExceptionalCondition(const char *conditionName,
do { _Static_assert(condition, errmessage); } while(0)
#define StaticAssertExpr(condition, errmessage) \
((void) ({ StaticAssertStmt(condition, errmessage); true; }))
+#define StaticAssertDecl(condition, errmessage) \
+ _Static_assert(condition, errmessage)
#else /* !HAVE__STATIC_ASSERT */
#define StaticAssertStmt(condition, errmessage) \
((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
#define StaticAssertExpr(condition, errmessage) \
StaticAssertStmt(condition, errmessage)
+#define StaticAssertDecl(condition, errmessage) \
+ extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1])
#endif /* HAVE__STATIC_ASSERT */
#else /* C++ */
#if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
@@ -857,12 +863,16 @@ extern void ExceptionalCondition(const char *conditionName,
static_assert(condition, errmessage)
#define StaticAssertExpr(condition, errmessage) \
({ static_assert(condition, errmessage); })
-#else
+#define StaticAssertDecl(condition, errmessage) \
+ static_assert(condition, errmessage)
+#else /* !__cpp_static_assert */
#define StaticAssertStmt(condition, errmessage) \
do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0)
#define StaticAssertExpr(condition, errmessage) \
((void) ({ StaticAssertStmt(condition, errmessage); }))
-#endif
+#define StaticAssertDecl(condition, errmessage) \
+ extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1])
+#endif /* __cpp_static_assert */
#endif /* C++ */
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 870ecb51b7..3f88683a05 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On Fri, Dec 20, 2019 at 01:08:47AM +, Smith, Peter wrote: > I updated the most recent patch (_5 from Michael) so it now has your > suggested error message rewording. Hmm. Based on the last messages, and this one in particular: https://www.postgresql.org/message-id/20191202155545.yzbfzuppjriti...@alap3.anarazel.de I am still seeing that a couple of points need an extra lookup: - Addition of a Decl() in at least one header of the core code. - Perhaps unifying the fallback implementation between C and C++, with a closer lookup in particular at StaticAssertStmt() and StaticAssertExpr(). Peter, could you look at that? In order to test the C++ portion, you could use this dummy C++ extension I have just published on my github account: https://github.com/michaelpq/pg_plugins/tree/master/blackhole_cplusplus That's a dirty 15-min hack, but that would be enough to check the C++ part with PGXS. For now, I am switching the patch as waiting on author for now. -- Michael signature.asc Description: PGP signature
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On Wed, Dec 04, 2019 at 09:54:47AM -0800, Andres Freund wrote: > Well, _Static_assert has an error message, so we got to pass > something. And having something like "array length mismatch", without > referring again to the variable, doesn't strike me as that bad. We could > of course just again pass the expression, this time stringified, but > that doesn't seem an improvement. Yeah, I would rather keep the second argument. I think that's also more helpful as it gives more flexibility to extension authors willing to make use of it. -- Michael signature.asc Description: PGP signature
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
Hello Michael, > In short, attached is an updated version of your patch which attempts to > solve that. I have tested this with some cplusplus stuff, and GCC for both > versions (static_assert is available in GCC >= 6, but a manual change of c.h > does the trick). > I have edited the patch a bit while on it, your assertions did not use > project-style grammar, the use of parenthesis was inconsistent (see relpath.c > for example), and pgindent has complained a bit. Thanks for your updates. ~~ Hello Andres, >> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1), >> + "LockTagTypeNames array inconsistency"); >> + > These error messages strike me as somewhat unhelpful. I'd probably just > reword them as "array length mismatch" or something like that. I updated the most recent patch (_5 from Michael) so it now has your suggested error message rewording. PSA patch _6 Kind Regards Peter Smith Fujitsu Australia ct_asserts_StaticAssertDecl_6.patch Description: ct_asserts_StaticAssertDecl_6.patch
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
-Original Message-
From: Andres Freund Sent: Tuesday, 3 December 2019 2:56 AM
> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1),
> + "LockTagTypeNames array inconsistency");
> +
>
>These error messages strike me as somewhat unhelpful. I'd probably just reword
>them as "array length mismatch" or something like that.
OK. I have no problem to modify all my current assertion messages to your
suggested text ("array length mismatch") if you think it is better.
Please correct me if I am wrong, but I didn't think the error message text is
of very great significance here because it is a compile-time issue meaning the
*only* person who would see the message is the 1 developer who accidentally
introduced a bug just moments beforehand. The compile will fail with a source
line number, and when the developer sees the StaticAssertDecl at that source
line the cause of the error is anyway self-evident by the condition parameter.
Kind Regards
--
Peter Smith
Fujitsu Australia
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
Hi, On 2019-12-04 10:09:28 +0100, Peter Eisentraut wrote: > On 2019-12-02 16:55, Andres Freund wrote: > > > +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1), > > > + "LockTagTypeNames array inconsistency"); > > > + > > These error messages strike me as somewhat unhelpful. I'd probably just > > reword them as "array length mismatch" or something like that. > > I'd prefer it if we could just get rid of the second argument and show the > actual expression in the error message, like run-time assertions work. Well, _Static_assert has an error message, so we got to pass something. And having something like "array length mismatch", without referring again to the variable, doesn't strike me as that bad. We could of course just again pass the expression, this time stringified, but that doesn't seem an improvement. Greetings, Andres Freund
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On 2019-12-04 15:16:25 +0900, Michael Paquier wrote: > On Mon, Dec 02, 2019 at 07:55:45AM -0800, Andres Freund wrote: > > Now that I'm back from vacation, I'll try to take a stab at b). It > > should definitely doable to use the same approach for StaticAssertStmt, > > the problematic case might be StaticAssertExpr. > > So you basically want to minimize the amount of code relying on > external compiler expressions? Sounds like a plan. At quick glance, > it seems that this should work. I haven't tested though. I'll wait > for what you come up with then. I don't know what you mean by "external compiler expressions"? > >> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1), > >> + "LockTagTypeNames array inconsistency"); > >> + > > > > These error messages strike me as somewhat unhelpful. I'd probably just > > reword them as "array length mismatch" or something like that. > > That's indeed better. Now I think that it is useful to have the > structure name in the error message as well, no? No. I think the cost of having the different error messages is much higher than the cost of not having the struct name in there. Note that you'll commonly get an error message including the actual source code for the offending expression. > > I think this patch ought to include at least one StaticAssertDecl in a > > header, to make sure we get that part right across compilers. E.g. the > > one in PageIsVerified()? > > No objections to have one, but I don't think that your suggestion is a > good choice. This static assertion is based on size_t and BLCKSZ, and > is located close to a code path where we have a specific logic based > on both things. Well, but it's a reliance that goes beyond that specific source code location > If in the future this code gets removed, then we'd likely miss to > remove the static assertion if they are separated across multiple > files. It'll never get removed. There's just plainly no way that we'd use a block size that's not a multiple of size_t. Greetings, Andres Freund
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On 2019-12-02 16:55, Andres Freund wrote: +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1), +"LockTagTypeNames array inconsistency"); + These error messages strike me as somewhat unhelpful. I'd probably just reword them as "array length mismatch" or something like that. I'd prefer it if we could just get rid of the second argument and show the actual expression in the error message, like run-time assertions work. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On Mon, Dec 02, 2019 at 07:55:45AM -0800, Andres Freund wrote:
> On 2019-11-29 11:11:25 +0900, Michael Paquier wrote:
>> diff --git a/src/include/c.h b/src/include/c.h
>> index 00e41ac546..91d6d50e76 100644
>> --- a/src/include/c.h
>> +++ b/src/include/c.h
>> [...]
>
> I think this a) needs an updated comment above, explaining this approach
> (note the explanation for the array approach) b) I still think we ought
> to work towards also using this implementation for StaticAssertStmt.
Sure. I was not completely sure which addition would be helpful
except than adding in the main comment lock that Decl() is useful at
file scope.
> Now that I'm back from vacation, I'll try to take a stab at b). It
> should definitely doable to use the same approach for StaticAssertStmt,
> the problematic case might be StaticAssertExpr.
So you basically want to minimize the amount of code relying on
external compiler expressions? Sounds like a plan. At quick glance,
it seems that this should work. I haven't tested though. I'll wait
for what you come up with then.
>> #else /* C++ */
>> #if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
>> @@ -857,12 +862,16 @@ extern void ExceptionalCondition(const char
>> *conditionName,
>> static_assert(condition, errmessage)
>> #define StaticAssertExpr(condition, errmessage) \
>> ({ static_assert(condition, errmessage); })
>>
>> [...]
>>
>> +#define StaticAssertDecl(condition, errmessage) \
>> +extern void static_assert_func(int static_assert_failure[(condition) ?
>> 1 : -1])
>> +#endif /*
>> __cpp_static_assert */
>> #endif /* C++ */
>
> I wonder if it's worth moving the fallback implementation into an else
> branch that's "unified" between C and C++.
I suspect that you would run into problems with StaticAssertExpr() and
StaticAssertStmt().
>> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1),
>> + "LockTagTypeNames array inconsistency");
>> +
>
> These error messages strike me as somewhat unhelpful. I'd probably just
> reword them as "array length mismatch" or something like that.
That's indeed better. Now I think that it is useful to have the
structure name in the error message as well, no?
> I think this patch ought to include at least one StaticAssertDecl in a
> header, to make sure we get that part right across compilers. E.g. the
> one in PageIsVerified()?
No objections to have one, but I don't think that your suggestion is a
good choice. This static assertion is based on size_t and BLCKSZ, and
is located close to a code path where we have a specific logic based
on both things. If in the future this code gets removed, then we'd
likely miss to remove the static assertion if they are separated
across multiple files.
--
Michael
signature.asc
Description: PGP signature
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
Hi,
On 2019-11-29 11:11:25 +0900, Michael Paquier wrote:
> On Wed, Nov 27, 2019 at 12:23:33PM +, Smith, Peter wrote:
> > * That is beyond the scope for what I wanted my patch to achieve; my
> > * use-cases are C code only.
I really don't think that's justification enough for having diverging
implementations, nor imcomplete coverage. Following that chain of
arguments we'd just end up with more and more cruft, without ever
actually cleaning anything up.
> diff --git a/src/include/c.h b/src/include/c.h
> index 00e41ac546..91d6d50e76 100644
> --- a/src/include/c.h
> +++ b/src/include/c.h
> @@ -845,11 +845,16 @@ extern void ExceptionalCondition(const char
> *conditionName,
> do { _Static_assert(condition, errmessage); } while(0)
> #define StaticAssertExpr(condition, errmessage) \
> ((void) ({ StaticAssertStmt(condition, errmessage); true; }))
> +/* StaticAssertDecl is suitable for use at file scope. */
> +#define StaticAssertDecl(condition, errmessage) \
> + _Static_assert(condition, errmessage)
> #else/*
> !HAVE__STATIC_ASSERT */
> #define StaticAssertStmt(condition, errmessage) \
> ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 :
> -1; }))
> #define StaticAssertExpr(condition, errmessage) \
> StaticAssertStmt(condition, errmessage)
> +#define StaticAssertDecl(condition, errmessage) \
> + extern void static_assert_func(int static_assert_failure[(condition) ?
> 1 : -1])
> #endif /*
> HAVE__STATIC_ASSERT */
I think this a) needs an updated comment above, explaining this approach
(note the explanation for the array approach) b) I still think we ought
to work towards also using this implementation for StaticAssertStmt.
Now that I'm back from vacation, I'll try to take a stab at b). It
should definitely doable to use the same approach for StaticAssertStmt,
the problematic case might be StaticAssertExpr.
> #else/* C++ */
> #if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
> @@ -857,12 +862,16 @@ extern void ExceptionalCondition(const char
> *conditionName,
> static_assert(condition, errmessage)
> #define StaticAssertExpr(condition, errmessage) \
> ({ static_assert(condition, errmessage); })
> -#else
> +#define StaticAssertDecl(condition, errmessage) \
> + static_assert(condition, errmessage)
> +#else/*
> !__cpp_static_assert */
> #define StaticAssertStmt(condition, errmessage) \
> do { struct static_assert_struct { int static_assert_failure :
> (condition) ? 1 : -1; }; } while(0)
> #define StaticAssertExpr(condition, errmessage) \
> ((void) ({ StaticAssertStmt(condition, errmessage); }))
> -#endif
> +#define StaticAssertDecl(condition, errmessage) \
> + extern void static_assert_func(int static_assert_failure[(condition) ?
> 1 : -1])
> +#endif /*
> __cpp_static_assert */
> #endif /* C++ */
I wonder if it's worth moving the fallback implementation into an else
branch that's "unified" between C and C++.
> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1),
> + "LockTagTypeNames array inconsistency");
> +
These error messages strike me as somewhat unhelpful. I'd probably just
reword them as "array length mismatch" or something like that.
I think this patch ought to include at least one StaticAssertDecl in a
header, to make sure we get that part right across compilers. E.g. the
one in PageIsVerified()?
Greetings,
Andres Freund
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On Wed, Nov 27, 2019 at 12:23:33PM +, Smith, Peter wrote:
> * That is beyond the scope for what I wanted my patch to achieve; my
> * use-cases are C code only.
Well, FWIW, I do have some extensions using __cplusplus and I am
pretty sure that I am not the only one with that. The thing is that
with your patch folks would not get any compilation failures *now*
because all the declarations of StaticAssertDecl() are added within
the .c files, but once a patch which includes a declaration in a
header, something very likely to happen, is merged then we head into
breaking suddenly the compilation of those modules. And that's not
nice. That's also a point raised by Andres upthread.
> I am happy if somebody else with more ability to test C++ properly
> wants to add the __cplusplus variant of the new macro.
In short, attached is an updated version of your patch which attempts
to solve that. I have tested this with some cplusplus stuff, and GCC
for both versions (static_assert is available in GCC >= 6, but a
manual change of c.h does the trick).
I have edited the patch a bit while on it, your assertions did not use
project-style grammar, the use of parenthesis was inconsistent (see
relpath.c for example), and pgindent has complained a bit.
Also, I am bumping the patch to next CF for now. Do others have
thoughts to share about this version? I would be actually fine to
commit that, even if the message generated for the fallback versions
is a bit crappy with a complain about a negative array size, but
that's not new to this patch as we use that as well with
StaticAssertStmt().
--
Michael
diff --git a/src/include/c.h b/src/include/c.h
index 00e41ac546..91d6d50e76 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -845,11 +845,16 @@ extern void ExceptionalCondition(const char *conditionName,
do { _Static_assert(condition, errmessage); } while(0)
#define StaticAssertExpr(condition, errmessage) \
((void) ({ StaticAssertStmt(condition, errmessage); true; }))
+/* StaticAssertDecl is suitable for use at file scope. */
+#define StaticAssertDecl(condition, errmessage) \
+ _Static_assert(condition, errmessage)
#else /* !HAVE__STATIC_ASSERT */
#define StaticAssertStmt(condition, errmessage) \
((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
#define StaticAssertExpr(condition, errmessage) \
StaticAssertStmt(condition, errmessage)
+#define StaticAssertDecl(condition, errmessage) \
+ extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1])
#endif /* HAVE__STATIC_ASSERT */
#else /* C++ */
#if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
@@ -857,12 +862,16 @@ extern void ExceptionalCondition(const char *conditionName,
static_assert(condition, errmessage)
#define StaticAssertExpr(condition, errmessage) \
({ static_assert(condition, errmessage); })
-#else
+#define StaticAssertDecl(condition, errmessage) \
+ static_assert(condition, errmessage)
+#else /* !__cpp_static_assert */
#define StaticAssertStmt(condition, errmessage) \
do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0)
#define StaticAssertExpr(condition, errmessage) \
((void) ({ StaticAssertStmt(condition, errmessage); }))
-#endif
+#define StaticAssertDecl(condition, errmessage) \
+ extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1])
+#endif /* __cpp_static_assert */
#endif /* C++ */
diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
index bc62c6eef7..e7e992eb85 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -36,6 +36,9 @@ const char *const LockTagTypeNames[] = {
"advisory"
};
+StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1),
+ "LockTagTypeNames array inconsistency");
+
/* This must match enum PredicateLockTargetType (predicate_internals.h) */
static const char *const PredicateLockTagTypeNames[] = {
"relation",
@@ -43,6 +46,9 @@ static const char *const PredicateLockTagTypeNames[] = {
"tuple"
};
+StaticAssertDecl(lengthof(PredicateLockTagTypeNames) == (PREDLOCKTAG_TUPLE + 1),
+ "PredicateLockTagTypeNames array inconsistency");
+
/* Working status for pg_lock_status */
typedef struct
{
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ba4edde71a..3dfffac88f 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -236,6 +236,9 @@ static const struct config_enum_entry bytea_output_options[] = {
{NULL, 0, false}
};
+StaticAssertDecl(lengthof(bytea_output_options) == (BYTEA_OUTPUT_HEX + 2),
+ "bytea_output_options array inconsistency");
+
/*
* We have different sets for client and server message level options because
* they sort slightly different (see "log" level), and because "fatal"/"panic"
@@ -281,6 +284,9 @@ static const struct config_enum_entry intervalstyle_options[]
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
> It seems to me that there is a good point to be consistent with the treatment > of StaticAssertStmt and StaticAssertExpr in c.h, which have fallback > implementations in *all* the configurations supported. Consistency is good, but: * That is beyond the scope for what I wanted my patch to achieve; my use-cases are C code only * It is too risky for me to simply cut/paste my C version of StaticAssertDecl and hope it will work OK for C++. It needs lots of testing because there seems evidence that bad things can happen. E.g. Peter Eisentraut wrote "if you're asking, why is the fallback implementation in C++ different from the one in C, then that's because the C variant didn't work in C++." ~ I am happy if somebody else with more ability to test C++ properly wants to add the __cplusplus variant of the new macro. Meanwhile, I've attached latest re-based version of this patch. Kind Regards. -- Peter Smith Fujitsu Australia ct_asserts_StaticAssertDecl_4.patch Description: ct_asserts_StaticAssertDecl_4.patch
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
Hi Andres, >>> As far as I can tell we should be able to use the prototype based approach >>> in all the cases where we currently use the "negative bit-field width" >>> approach? >> ... >> But I did not refactor existing code to use the new way because I was >> fearful that there might be some subtle reason why the >> StaticAssertStmt was deliberately made that way (e.g. as do/while), >> and last thing I want to do was break working code. >That'll just leave us with cruft. And it's not like this stuff will break in a >subtle way or such FYI - I did try, per your suggestion, to replace the existing StaticAssertStmt to also use the same fallback "extern" syntax form as the new StaticAssertDecl, but the code broke as I suspected it might do: path.c: In function 'first_dir_separator': ../../src/include/c.h:847:2: error: expected expression before 'extern' extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1]) ^ ../../src/include/c.h:849:2: note: in expansion of macro 'StaticAssertStmt' StaticAssertStmt(condition, errmessage) ^ ../../src/include/c.h:1184:3: note: in expansion of macro 'StaticAssertExpr' (StaticAssertExpr(__builtin_types_compatible_p(__typeof(expr), const underlying_type), \ ^ path.c:127:11: note: in expansion of macro 'unconstify' return unconstify(char *, p); ^ Kind Regards. --- Peter Smith Fujitsu Australia
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
Hi, On 2019-11-13 03:23:06 +, Smith, Peter wrote: > From: Andres Freund Sent: Wednesday, 13 November 2019 > 6:01 AM > > >Peter Smith: > > > > Is there a reason to not just make StaticAssertExpr and StaticAssertStmt be > > the same? I don't want to proliferate variants that users have to > > understand if there's no compelling > > need. Nor do I think do we really need two different fallback > > implementation for static asserts. > > > > > As far as I can tell we should be able to use the prototype based approach > > in all the cases where we currently use the "negative bit-field width" > > approach? > > I also thought that the new "prototype negative array-dimension" based > approach (i.e. StaticAssertDecl) looked like an improvement over the > existing "negative bit-field width" approach (i.e. StaticAssertStmt), > because it seems to work for more scenarios (e.g. file scope). > > But I did not refactor existing code to use the new way because I was > fearful that there might be some subtle reason why the > StaticAssertStmt was deliberately made that way (e.g. as do/while), > and last thing I want to do was break working code. That'll just leave us with cruft. And it's not like this stuff will break in a subtle way or such Greetings, Andres Freund
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On 2019-11-12 20:00, Andres Freund wrote: Looking at the cplusplus variant, I'm somewhat surprised to see that you made both fallback and plain version unconditionally use GCC style compound expressions: Was that intentional? The C version intentionally uses compound expressions only for the _Static_assert case, where configure tests for the compound expression support? As far as I can tell this'll not allow using our headers e.g. with msvc in C++ mode if somebody introduce a static assertion in a header - which seems like a likely and good outcome with the changes proposed here? I don't recall all the details anymore, but if you're asking, why is the fallback implementation in C++ different from the one in C, then that's because the C variant didn't work in C++. I seem to recall that I did this work in order to get an actual C++-using extension to compile, so it worked(tm) at some point, but I probably didn't try it with a not-gcc compatible compiler at the time. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
From: Andres Freund Sent: Wednesday, 13 November 2019 6:01 AM >Peter Smith: > > Is there a reason to not just make StaticAssertExpr and StaticAssertStmt be > the same? I don't want to proliferate variants that users have to understand > if there's no compelling > need. Nor do I think do we really need two different fallback implementation > for static asserts. > > As far as I can tell we should be able to use the prototype based approach in > all the cases where we currently use the "negative bit-field width" approach? I also thought that the new "prototype negative array-dimension" based approach (i.e. StaticAssertDecl) looked like an improvement over the existing "negative bit-field width" approach (i.e. StaticAssertStmt), because it seems to work for more scenarios (e.g. file scope). But I did not refactor existing code to use the new way because I was fearful that there might be some subtle reason why the StaticAssertStmt was deliberately made that way (e.g. as do/while), and last thing I want to do was break working code. > Should then also update > * Otherwise we fall back on a kluge that assumes the compiler will complain > * about a negative width for a struct bit-field. This will not include a > * helpful error message, but it beats not getting an error at all. Kind Regards. Peter Smith --- Fujitsu Australia
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
Hi Peter, Peter, :)
On 2019-10-28 00:30:11 +, Smith, Peter wrote:
> From: Andres Freund Sent: Sunday, 27 October 2019 12:03
> PM
> > My proposal for this really was just to use this as a fallback for when
> > static assert isn't available. Which in turn I was just suggesting because
> > Tom wanted a fallback.
>
> The patch is updated to use "extern" technique only when "_Static_assert" is
> unavailable.
Cool.
> /*
> * forkname_to_number - look up fork number by name
> diff --git a/src/include/c.h b/src/include/c.h
> index d752cc0..3e24ff4 100644
> --- a/src/include/c.h
> +++ b/src/include/c.h
> @@ -838,11 +838,16 @@ extern void ExceptionalCondition(const char
> *conditionName,
> do { _Static_assert(condition, errmessage); } while(0)
> #define StaticAssertExpr(condition, errmessage) \
> ((void) ({ StaticAssertStmt(condition, errmessage); true; }))
> +/* StaticAssertDecl is suitable for use at file scope. */
> +#define StaticAssertDecl(condition, errmessage) \
> + _Static_assert(condition, errmessage)
> #else/*
> !HAVE__STATIC_ASSERT */
> #define StaticAssertStmt(condition, errmessage) \
> ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 :
> -1; }))
> #define StaticAssertExpr(condition, errmessage) \
> StaticAssertStmt(condition, errmessage)
> +#define StaticAssertDecl(condition, errmessage) \
> + extern void static_assert_func(int static_assert_failure[(condition) ?
> 1 : -1])
> #endif /*
> HAVE__STATIC_ASSERT */
> #else/* C++ */
> #if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
> @@ -858,7 +863,6 @@ extern void ExceptionalCondition(const char
> *conditionName,
> #endif
> #endif /* C++
> */
Peter Smith:
Is there a reason to not just make StaticAssertExpr and StaticAssertStmt
be the same? I don't want to proliferate variants that users have to
understand if there's no compelling need. Nor do I think do we really
need two different fallback implementation for static asserts.
As far as I can tell we should be able to use the prototype based
approach in all the cases where we currently use the "negative bit-field
width" approach?
Should then also update
* Otherwise we fall back on a kluge that assumes the compiler will complain
* about a negative width for a struct bit-field. This will not include a
* helpful error message, but it beats not getting an error at all.
Peter Eisentraut:
Looking at the cplusplus variant, I'm somewhat surprised to see that you
made both fallback and plain version unconditionally use GCC style
compound expressions:
commit a2c8e5cfdb9d82ae6d4bb8f37a4dc7cbeca63ec1
Author: Peter Eisentraut
Date: 2016-08-30 12:00:00 -0400
Add support for static assertions in C++
...
+#if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
+#define StaticAssertStmt(condition, errmessage) \
+static_assert(condition, errmessage)
+#define StaticAssertExpr(condition, errmessage) \
+StaticAssertStmt(condition, errmessage)
+#else
+#define StaticAssertStmt(condition, errmessage) \
+do { struct static_assert_struct { int static_assert_failure : (condition)
? 1 : -1; }; } while(0)
+#define StaticAssertExpr(condition, errmessage) \
+({ StaticAssertStmt(condition, errmessage); })
+#endif
Was that intentional? The C version intentionally uses compound
expressions only for the _Static_assert case, where configure tests for
the compound expression support? As far as I can tell this'll not allow
using our headers e.g. with msvc in C++ mode if somebody introduce a
static assertion in a header - which seems like a likely and good
outcome with the changes proposed here?
Btw, it looks to me like msvc supports using the C++ static_assert()
even in C mode:
https://godbolt.org/z/b_dxDW
Greetings,
Andres Freund
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On Mon, Oct 28, 2019 at 03:42:02AM +, Smith, Peter wrote: > From: Kyotaro Horiguchi Sent: Monday, 28 October > 2019 1:26 PM >> It is missing the __cplusplus case? > > My use cases for the macro are only in C code, so that's all I was interested > in at this time. > If somebody else wants to extend the patch for C++ also (and test it) they > can do. It seems to me that there is a good point to be consistent with the treatment of StaticAssertStmt and StaticAssertExpr in c.h, which have fallback implementations in *all* the configurations supported. @@ -858,7 +863,6 @@ extern void ExceptionalCondition(const char *conditionName, #endif #endif /* C++ */ - /* A nit: noise diffs. (No need to send a new version just for that.) -- Michael signature.asc Description: PGP signature
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
From: Kyotaro Horiguchi Sent: Monday, 28 October 2019 1:26 PM > It is missing the __cplusplus case? My use cases for the macro are only in C code, so that's all I was interested in at this time. If somebody else wants to extend the patch for C++ also (and test it) they can do. Kind Regards, --- Peter Smith Fujitsu Australia
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
From: Andres Freund Sent: Sunday, 27 October 2019 12:03 PM > My proposal for this really was just to use this as a fallback for when > static assert isn't available. Which in turn I was just suggesting because > Tom wanted a fallback. The patch is updated to use "extern" technique only when "_Static_assert" is unavailable. PSA. Kind Regards, --- Peter Smith Fujitsu Australia ct_asserts_StaticAssertDecl_3.patch Description: ct_asserts_StaticAssertDecl_3.patch
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On 2019-10-27 11:44:54 +, Smith, Peter wrote: > From: Andres Freund Sent: Sunday, 27 October 2019 12:03 > PM > > I think it should work everywhere that has static assert. So we should need > > a separate configure check. > > Er, that's a typo right? I think you meant: "So we *shouldn't* need a > separate configure check" Yes.
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
From: Andres Freund Sent: Sunday, 27 October 2019 12:03 PM >>Ideally, the implementation should end up calling _Static_assert() >>somehow, so that we get the compiler's native error message. OK. I can work on that. >>We could do a configure check for whether _Static_assert() works at >>file scope. I don't know what the support for that is, but it seems to >>work in gcc and clang > I think it should work everywhere that has static assert. So we should need a > separate configure check. Er, that's a typo right? I think you meant: "So we *shouldn't* need a separate configure check" Kind Regards --- Peter Smith Fujitsu Australia
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
Hi, On October 26, 2019 6:06:07 AM PDT, Peter Eisentraut wrote: >On 2019-10-10 00:52, Smith, Peter wrote: >> I liked your idea of using an extern function declaration for >implementing the file-scope compile-time asserts. AFAIK it is valid >standard C. >> >> Thank you for the useful link to that compiler explorer. I tried many >scenarios of the new StaticAssertDecl and all seemed to work ok. >> https://godbolt.org/z/fDrmXi >> >> The patch has been updated accordingly. All assertions identified in >the original post are now adjacent the global variables they are >asserting. >> > >The problem with this implementation is that you get a crappy error >message when the assertion fails, namely something like: > >../../../../src/include/c.h:862:84: error: size of array >'static_assert_failure' is negative My proposal for this really was just to use this as a fallback for when static assert isn't available. Which in turn I was just suggesting because Tom wanted a fallback. >Ideally, the implementation should end up calling _Static_assert() >somehow, so that we get the compiler's native error message. > >We could do a configure check for whether _Static_assert() works at >file >scope. I don't know what the support for that is, but it seems to work >in gcc and clang. I think it should work everywhere that has static assert. So we should need a separate configure check. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On 2019-10-10 00:52, Smith, Peter wrote: > I liked your idea of using an extern function declaration for implementing > the file-scope compile-time asserts. AFAIK it is valid standard C. > > Thank you for the useful link to that compiler explorer. I tried many > scenarios of the new StaticAssertDecl and all seemed to work ok. > https://godbolt.org/z/fDrmXi > > The patch has been updated accordingly. All assertions identified in the > original post are now adjacent the global variables they are asserting. > The problem with this implementation is that you get a crappy error message when the assertion fails, namely something like: ../../../../src/include/c.h:862:84: error: size of array 'static_assert_failure' is negative Ideally, the implementation should end up calling _Static_assert() somehow, so that we get the compiler's native error message. We could do a configure check for whether _Static_assert() works at file scope. I don't know what the support for that is, but it seems to work in gcc and clang. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
From: Andres Freund Sent: Tuesday, 1 October 2019 3:14 AM >I wonder if defining the fallback static assert code to something like > extern void static_assert_func(int static_assert_failed[(condition) ? 1 : > -1]); isn't a solution, however. I *think* that's standard C. Seems to work > at least with gcc, clang, msvc, icc. > >Re standard: C99's "6.7 Declarations" + 6.7.1 defines 'declaration' to include >extern specifiers and in 6.7.1 5) says "The declaration of an identifier for a >function that has block scope shall have >no explicit storage-class specifier >other than extern.". And "6.8 Statements and blocks", via "6.8.2 Compound >statement" allows declarations in statements. > >You can play with a good few compilers at: https://godbolt.org/z/fl0Mzu I liked your idea of using an extern function declaration for implementing the file-scope compile-time asserts. AFAIK it is valid standard C. Thank you for the useful link to that compiler explorer. I tried many scenarios of the new StaticAssertDecl and all seemed to work ok. https://godbolt.org/z/fDrmXi The patch has been updated accordingly. All assertions identified in the original post are now adjacent the global variables they are asserting. Kind Regards -- Peter Smith Fujitsu Australia ct_asserts_StaticAssertDecl_2.patch Description: ct_asserts_StaticAssertDecl_2.patch
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
Hi, On September 30, 2019 10:20:54 AM PDT, Tom Lane wrote: >Andres Freund writes: >> On 2019-09-19 04:46:27 +, Smith, Peter wrote: >>> In the attached patch example I have defined a new macro >>> StaticAssertDecl. A typical usage of it is shown in the relpath.c >>> file. > >> I'm in favor of adding that - in fact, when I was working on adding a >a >> static_assert wrapper, I was advocating for only supporting compilers >> that know static_assert, so we can put these in the global scope. The >> number of compilers that don't support static_assert is pretty small >> today, especially compared to 2012, when we added these. >> >https://www.postgresql.org/message-id/E1TIW1p-0002yE-AY%40gemulon.postgresql.org >> >https://www.postgresql.org/message-id/27446.1349024252%40sss.pgh.pa.us >> Tom, you were arguing for restricting to file scope to get broader >> compatibility, are you ok with adding a seperate *Decl version? > >It could use another look now that we require C99. I'd be in favor >of having a decl-level static assert if practical. What do you think about my proposal further down in the email to rely on extern function declarations to have one fallback that works in the relevant scopes (not in expressions, but we already treat that differently)? Seems to work on common compilers and seems standard conform? Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
Andres Freund writes: > On 2019-09-19 04:46:27 +, Smith, Peter wrote: >> In the attached patch example I have defined a new macro >> StaticAssertDecl. A typical usage of it is shown in the relpath.c >> file. > I'm in favor of adding that - in fact, when I was working on adding a a > static_assert wrapper, I was advocating for only supporting compilers > that know static_assert, so we can put these in the global scope. The > number of compilers that don't support static_assert is pretty small > today, especially compared to 2012, when we added these. > https://www.postgresql.org/message-id/E1TIW1p-0002yE-AY%40gemulon.postgresql.org > https://www.postgresql.org/message-id/27446.1349024252%40sss.pgh.pa.us > Tom, you were arguing for restricting to file scope to get broader > compatibility, are you ok with adding a seperate *Decl version? It could use another look now that we require C99. I'd be in favor of having a decl-level static assert if practical. regards, tom lane
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
Hi, On 2019-09-19 04:46:27 +, Smith, Peter wrote: > In the attached patch example I have defined a new macro > StaticAssertDecl. A typical usage of it is shown in the relpath.c > file. I'm in favor of adding that - in fact, when I was working on adding a a static_assert wrapper, I was advocating for only supporting compilers that know static_assert, so we can put these in the global scope. The number of compilers that don't support static_assert is pretty small today, especially compared to 2012, when we added these. https://www.postgresql.org/message-id/E1TIW1p-0002yE-AY%40gemulon.postgresql.org https://www.postgresql.org/message-id/27446.1349024252%40sss.pgh.pa.us Tom, you were arguing for restricting to file scope to get broader compatibility, are you ok with adding a seperate *Decl version? Or perhaps it's time to just remove the fallback implementation? I think we'd have to add proper MSVC support, but that seems like something we should do anyway. Back then I was wondering about using tyepedef to emulate static assert that works both in file and block scope, but that struggles with needing unique names. FWIW, the perl5 implementation has precisely that problem. If it's used in multiple headers (or a header and a .c file), two static asserts may not be on the same line... - which one will only notice when using an old compiler. I wonder if defining the fallback static assert code to something like extern void static_assert_func(int static_assert_failed[(condition) ? 1 : -1]); isn't a solution, however. I *think* that's standard C. Seems to work at least with gcc, clang, msvc, icc. Re standard: C99's "6.7 Declarations" + 6.7.1 defines 'declaration' to include extern specifiers and in 6.7.1 5) says "The declaration of an identifier for a function that has block scope shall have no explicit storage-class specifier other than extern.". And "6.8 Statements and blocks", via "6.8.2 Compound statement" allows declarations in statements. You can play with a good few compilers at: https://godbolt.org/z/fl0Mzu > The goal was to leave all existing Postgres static assert macros unchanged, > but to allow static asserts to be added in the code at file scope without the > need for the explicit ct_asserts function. It'd probably worthwhile to move many of the current ones. > Notice, in reality the StaticAssertDecl macro still uses a static function as > a wrapper for the StaticAssertStmt, but now the function is not visible in > the source. I think this implementation is not ok, due to the unique-name issue. Greetings, Andres Freund
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
-Original Message- From: Michael Paquier Sent: Thursday, 19 September 2019 11:08 AM >On Wed, Sep 18, 2019 at 04:46:30PM +0100, Dagfinn Ilmari Mannsåker wrote: >> Postgres doesn't seem to have it, but it would be possible to define a >> StaticAssertDecl macro that can be used at the file level, outside any >> function. See for example Perl's STATIC_ASSERT_DECL: >> >> https://github.com/Perl/perl5/blob/v5.30.0/perl.h#L3455-L3488 > >That sounds like a cleaner alternative. Thanks for the pointer. In the attached patch example I have defined a new macro StaticAssertDecl. A typical usage of it is shown in the relpath.c file. The goal was to leave all existing Postgres static assert macros unchanged, but to allow static asserts to be added in the code at file scope without the need for the explicit ct_asserts function. Notice, in reality the StaticAssertDecl macro still uses a static function as a wrapper for the StaticAssertStmt, but now the function is not visible in the source. If this strategy is acceptable I will update my original patch to remove all those ct_asserts functions, and instead put each StaticAssertDecl nearby the array that it is asserting (e.g. just like relpath.c) Kind Regards, Peter Smith --- Fujitsu Australia add_more_ct_asserts_StaticAssertDecl.patch Description: add_more_ct_asserts_StaticAssertDecl.patch
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
At Thu, 19 Sep 2019 10:07:40 +0900, Michael Paquier wrote
in <20190919010740.gc22...@paquier.xyz>
> On Wed, Sep 18, 2019 at 04:46:30PM +0100, Dagfinn Ilmari Mannsåker wrote:
> > Postgres doesn't seem to have it, but it would be possible to define a
> > StaticAssertDecl macro that can be used at the file level, outside any
> > function. See for example Perl's STATIC_ASSERT_DECL:
> >
> > https://github.com/Perl/perl5/blob/v5.30.0/perl.h#L3455-L3488
>
> That sounds like a cleaner alternative. Thanks for the pointer.
The cause for StaticAssertStmt not being usable outside of
functions is enclosing do-while, which is needed to avoid "mixed
declaration" warnings, which we are inhibiting to use as of
now. Therefore just defining another macro defined as just
_Static_assert() works fine.
I don't find an alternative way for the tool chains that don't
have static assertion feature. In the attached diff the macro is
defined as nothing. I don't find a way to warn that the assertion
is ignored.
regards.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 90ffd89339..822b9846bf 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4601,7 +4601,6 @@ static void write_auto_conf_file(int fd, const char *filename, ConfigVariable *h
static void replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
const char *name, const char *value);
-
/*
* Some infrastructure for checking malloc/strdup/realloc calls
*/
diff --git a/src/include/c.h b/src/include/c.h
index f461628a24..a386a81a19 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -836,11 +836,14 @@ extern void ExceptionalCondition(const char *conditionName,
#ifdef HAVE__STATIC_ASSERT
#define StaticAssertStmt(condition, errmessage) \
do { _Static_assert(condition, errmessage); } while(0)
+#define StaticAssertDecl(condition, errmessage) \
+ _Static_assert(condition, errmessage)
#define StaticAssertExpr(condition, errmessage) \
((void) ({ StaticAssertStmt(condition, errmessage); true; }))
#else /* !HAVE__STATIC_ASSERT */
#define StaticAssertStmt(condition, errmessage) \
((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
+#define StaticAssertDecl(condition, errmessage) /* no alternatives exist */
#define StaticAssertExpr(condition, errmessage) \
StaticAssertStmt(condition, errmessage)
#endif /* HAVE__STATIC_ASSERT */
@@ -848,11 +851,14 @@ extern void ExceptionalCondition(const char *conditionName,
#if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
#define StaticAssertStmt(condition, errmessage) \
static_assert(condition, errmessage)
+#define StaticAssertDecl(condition, errmessage) \
+ static_assert(condition, errmessage)
#define StaticAssertExpr(condition, errmessage) \
({ static_assert(condition, errmessage); })
#else
#define StaticAssertStmt(condition, errmessage) \
do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0)
+#define StaticAssertDecl(condition, errmessage) /* no alternatives exist */
#define StaticAssertExpr(condition, errmessage) \
((void) ({ StaticAssertStmt(condition, errmessage); }))
#endif
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On Wed, Sep 18, 2019 at 04:46:30PM +0100, Dagfinn Ilmari Mannsåker wrote: > Postgres doesn't seem to have it, but it would be possible to define a > StaticAssertDecl macro that can be used at the file level, outside any > function. See for example Perl's STATIC_ASSERT_DECL: > > https://github.com/Perl/perl5/blob/v5.30.0/perl.h#L3455-L3488 That sounds like a cleaner alternative. Thanks for the pointer. -- Michael signature.asc Description: PGP signature
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
-Original Message- From: Michael Paquier Sent: Wednesday, 18 September 2019 5:40 PM > For some of them it could help, and we could think about a better location > for that stuff than an unused routine. The indentation of your patch is > weird, with "git diff --check" complaining a lot. > > If you want to discuss more about that, could you add that to the next commit > fest? Here it is: https://commitfest.postgresql.org/25/ Hi Michael, Thanks for your feedback and advice. I have modified the patch to clean up the whitespace issues, and added it to the next commit fest. Kind Regards, --- Peter Smith Fujitsu Australia add_more_ct_asserts.patch Description: add_more_ct_asserts.patch
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
Michael Paquier writes: > On Wed, Sep 18, 2019 at 06:46:24AM +, Smith, Peter wrote: >> I have identified some OSS code where more compile-time asserts could be >> added. >> >> Mostly these are asserting that arrays have the necessary length to >> accommodate the enums that are used to index into them. >> >> In general the code is already commented with warnings such as: >> * "If you add a new entry, remember to ..." >> * "When modifying this enum, update the table in ..." >> * "Display names for enums in ..." >> * etc. >> >> But comments can be accidentally overlooked, so adding the >> compile-time asserts can help eliminate human error. > > For some of them it could help, and we could think about a better > location for that stuff than an unused routine. Postgres doesn't seem to have it, but it would be possible to define a StaticAssertDecl macro that can be used at the file level, outside any function. See for example Perl's STATIC_ASSERT_DECL: https://github.com/Perl/perl5/blob/v5.30.0/perl.h#L3455-L3488 - ilmari -- "The surreality of the universe tends towards a maximum" -- Skud's Law "Never formulate a law or axiom that you're not prepared to live with the consequences of." -- Skud's Meta-Law
Re: Proposal: Add more compile-time asserts to expose inconsistencies.
On Wed, Sep 18, 2019 at 06:46:24AM +, Smith, Peter wrote: > I have identified some OSS code where more compile-time asserts could be > added. > > Mostly these are asserting that arrays have the necessary length to > accommodate the enums that are used to index into them. > > In general the code is already commented with warnings such as: > * "If you add a new entry, remember to ..." > * "When modifying this enum, update the table in ..." > * "Display names for enums in ..." > * etc. > > But comments can be accidentally overlooked, so adding the > compile-time asserts can help eliminate human error. For some of them it could help, and we could think about a better location for that stuff than an unused routine. The indentation of your patch is weird, with "git diff --check" complaining a lot. If you want to discuss more about that, could you add that to the next commit fest? Here it is: https://commitfest.postgresql.org/25/ -- Michael signature.asc Description: PGP signature
Proposal: Add more compile-time asserts to expose inconsistencies.
Dear Hackers, I have identified some OSS code where more compile-time asserts could be added. Mostly these are asserting that arrays have the necessary length to accommodate the enums that are used to index into them. In general the code is already commented with warnings such as: * "If you add a new entry, remember to ..." * "When modifying this enum, update the table in ..." * "Display names for enums in ..." * etc. But comments can be accidentally overlooked, so adding the compile-time asserts can help eliminate human error. Please refer to the attached patch. Kind Regards, Peter Smith --- Fujitsu Australia 0001-Add-compile-time-asserts.patch Description: 0001-Add-compile-time-asserts.patch
