Proposal: Add more compile-time asserts to expose inconsistencies.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Proposal: Add more compile-time asserts to expose inconsistencies.

Smith, Peter
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 (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Dagfinn Ilmari Mannsåker
Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

RE: Proposal: Add more compile-time asserts to expose inconsistencies.

Smith, Peter
In reply to this post by Michael Paquier-2
-----Original Message-----
From: Michael Paquier <[hidden email]>  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 (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Michael Paquier-2
In reply to this post by Dagfinn Ilmari Mannsåker
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Kyotaro Horiguchi-4
At Thu, 19 Sep 2019 10:07:40 +0900, Michael Paquier <[hidden email]> wrote in <[hidden email]>
> 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
Reply | Threaded
Open this post in threaded view
|

RE: Proposal: Add more compile-time asserts to expose inconsistencies.

Smith, Peter
In reply to this post by Michael Paquier-2
-----Original Message-----
From: Michael Paquier <[hidden email]> 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 (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Andres Freund
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.

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


Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Tom Lane-2
Andres Freund <[hidden email]> 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.
> 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


Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Andres Freund
Hi,

On September 30, 2019 10:20:54 AM PDT, Tom Lane <[hidden email]> wrote:

>Andres Freund <[hidden email]> 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.
>>
>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.


Reply | Threaded
Open this post in threaded view
|

RE: Proposal: Add more compile-time asserts to expose inconsistencies.

Smith, Peter
In reply to this post by Andres Freund
From: Andres Freund <[hidden email]> 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 (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Andres Freund
Hi,

On October 26, 2019 6:06:07 AM PDT, Peter Eisentraut <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

RE: Proposal: Add more compile-time asserts to expose inconsistencies.

Smith, Peter
From: Andres Freund <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Andres Freund
On 2019-10-27 11:44:54 +0000, Smith, Peter wrote:
> From: Andres Freund <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

RE: Proposal: Add more compile-time asserts to expose inconsistencies.

Smith, Peter
In reply to this post by Andres Freund
From: Andres Freund <[hidden email]> 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 (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Kyotaro Horiguchi-4
Hello.

At Mon, 28 Oct 2019 00:30:11 +0000, "Smith, Peter" <[hidden email]> wrote in
> From: Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

RE: Proposal: Add more compile-time asserts to expose inconsistencies.

Smith, Peter
From: Kyotaro Horiguchi <[hidden email]> 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




Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Michael Paquier-2
On Mon, Oct 28, 2019 at 03:42:02AM +0000, Smith, Peter wrote:
> From: Kyotaro Horiguchi <[hidden email]> 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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

Andres Freund
In reply to this post by Smith, Peter
Hi Peter, Peter, :)


On 2019-10-28 00:30:11 +0000, Smith, Peter wrote:
> From: Andres Freund <[hidden email]> 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 <[hidden email]>
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


12