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
Attachments:
0001-Add-compile-time-asserts.patchapplication/octet-stream; name=0001-Add-compile-time-asserts.patchDownload+66-1
On Wed, Sep 18, 2019 at 06:46:24AM +0000, 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
Michael Paquier <michael@paquier.xyz> writes:
On Wed, Sep 18, 2019 at 06:46:24AM +0000, 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
-----Original Message-----
From: Michael Paquier <michael@paquier.xyz> 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
Attachments:
add_more_ct_asserts.patchapplication/octet-stream; name=add_more_ct_asserts.patchDownload+65-0
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
At Thu, 19 Sep 2019 10:07:40 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190919010740.GC22307@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.
Attachments:
define_StaticAssertDecl.patchtext/x-patch; charset=us-asciiDownload+6-1
-----Original Message-----
From: Michael Paquier <michael@paquier.xyz> 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
Attachments:
add_more_ct_asserts_StaticAssertDecl.patchapplication/octet-stream; name=add_more_ct_asserts_StaticAssertDecl.patchDownload+71-0
Hi,
On 2019-09-19 04:46:27 +0000, 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.
/messages/by-id/E1TIW1p-0002yE-AY@gemulon.postgresql.org
/messages/by-id/27446.1349024252@sss.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
Andres Freund <andres@anarazel.de> writes:
On 2019-09-19 04:46:27 +0000, 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.
/messages/by-id/E1TIW1p-0002yE-AY@gemulon.postgresql.org
/messages/by-id/27446.1349024252@sss.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
Hi,
On September 30, 2019 10:20:54 AM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
On 2019-09-19 04:46:27 +0000, 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./messages/by-id/E1TIW1p-0002yE-AY@gemulon.postgresql.org
/messages/by-id/27446.1349024252@sss.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.
From: Andres Freund <andres@anarazel.de> 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
Attachments:
ct_asserts_StaticAssertDecl_2.patchapplication/octet-stream; name=ct_asserts_StaticAssertDecl_2.patchDownload+36-0
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/fDrmXiThe 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
Hi,
On October 26, 2019 6:06:07 AM PDT, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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.
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.
From: Andres Freund <andres@anarazel.de> 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
On 2019-10-27 11:44:54 +0000, Smith, Peter wrote:
From: Andres Freund <andres@anarazel.de> 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.
From: Andres Freund <andres@anarazel.de> 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
Attachments:
ct_asserts_StaticAssertDecl_3.patchapplication/octet-stream; name=ct_asserts_StaticAssertDecl_3.patchDownload+39-1
Hello.
At Mon, 28 Oct 2019 00:30:11 +0000, "Smith, Peter" <peters@fast.au.fujitsu.com> wrote in
From: Andres Freund <andres@anarazel.de> 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.
It is missing the __cplusplus case?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> 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
On Mon, Oct 28, 2019 at 03:42:02AM +0000, Smith, Peter wrote:
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> 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
Hi Peter, Peter, :)
On 2019-10-28 00:30:11 +0000, Smith, Peter wrote:
From: Andres Freund <andres@anarazel.de> 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 <peter_e@gmx.net>
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); })
+#endifWas 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