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

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

From
"Smith, Peter"
Date:
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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Michael Paquier
Date:
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

Attachment

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

From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
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



RE: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
"Smith, Peter"
Date:
-----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.
Theindentation 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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Michael Paquier
Date:
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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Kyotaro Horiguchi
Date:
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.
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 exposeinconsistencies.

From
"Smith, Peter"
Date:
-----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.cfile. 

The goal was to leave all existing Postgres static assert macros unchanged, but to allow static asserts to be added in
thecode 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
nowthe 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
puteach StaticAssertDecl nearby the array that it is asserting (e.g. just like relpath.c) 

Kind Regards,
Peter Smith
---
Fujitsu Australia

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Andres Freund
Date:
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
inthe 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,
butnow 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.

From
Tom Lane
Date:
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.
> 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.

From
Andres Freund
Date:
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.
>>
>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
fallbackthat works in the relevant scopes (not in expressions, but we already treat that differently)? Seems to work on
commoncompilers 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 exposeinconsistencies.

From
"Smith, Peter"
Date:
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'sstandard 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
"Thedeclaration of an identifier for a function that has block scope shall have >no explicit storage-class specifier
otherthan 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
itis valid standard C. 

Thank you for the useful link to that compiler explorer. I tried many scenarios of the new StaticAssertDecl and all
seemedto 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
variablesthey are asserting.  

Kind Regards
--
Peter Smith
Fujitsu Australia

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Peter Eisentraut
Date:
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
itis valid standard C.
 
> 
> Thank you for the useful link to that compiler explorer. I tried many scenarios of the new StaticAssertDecl and all
seemedto 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
variablesthey 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
Date:
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.
>> 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
wasjust 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 exposeinconsistencies.

From
"Smith, Peter"
Date:
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

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Andres Freund
Date:
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.



RE: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
"Smith, Peter"
Date:
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
Iwas 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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Kyotaro Horiguchi
Date:
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
turnI 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



RE: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
"Smith, Peter"
Date:
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





Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Michael Paquier
Date:
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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Andres Freund
Date:
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
turnI 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); })
+#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 exposeinconsistencies.

From
"Smith, Peter"
Date:
From: Andres Freund <andres@anarazel.de> 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
variantsthat 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
improvementover the existing "negative bit-field width" approach (i.e. StaticAssertStmt), because it seems to work for
morescenarios (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
whythe StaticAssertStmt was deliberately made that way (e.g. as do/while), and last thing I want to do was break
workingcode. 

> 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 exposeinconsistencies.

From
Peter Eisentraut
Date:
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 exposeinconsistencies.

From
Andres Freund
Date:
Hi,

On 2019-11-13 03:23:06 +0000, Smith, Peter wrote:
> From: Andres Freund <andres@anarazel.de> 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
variantsthat 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 exposeinconsistencies.

From
"Smith, Peter"
Date:
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"
syntaxform 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 exposeinconsistencies.

From
"Smith, Peter"
Date:
> It seems to me that there is a good point to be consistent with the treatment of StaticAssertStmt and
StaticAssertExprin 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
needslots 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
workin 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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Michael Paquier
Date:
On Wed, Nov 27, 2019 at 12:23:33PM +0000, 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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Andres Freund
Date:
Hi,

On 2019-11-29 11:11:25 +0900, Michael Paquier wrote:
> On Wed, Nov 27, 2019 at 12:23:33PM +0000, 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 exposeinconsistencies.

From
Michael Paquier
Date:
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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Peter Eisentraut
Date:
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 exposeinconsistencies.

From
Andres Freund
Date:
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 exposeinconsistencies.

From
Andres Freund
Date:
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 exposeinconsistencies.

From
"Smith, Peter"
Date:
-----Original Message-----
From: Andres Freund <andres@anarazel.de> 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
somethinglike that. 

OK.  I have no problem to modify all my current assertion messages to your suggested text ("array length mismatch") if
youthink 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
itis a compile-time issue meaning the *only* person who would see the message is the 1 developer who accidentally
introduceda bug just moments beforehand. The compile will fail with a source line number, and when the developer sees
theStaticAssertDecl 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 exposeinconsistencies.

From
"Smith, Peter"
Date:
Hello Michael,

> In short, attached is an updated version of your patch which attempts to solve that.  I have tested this with some
cplusplusstuff, 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
wasinconsistent (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
somethinglike 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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Michael Paquier
Date:
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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Michael Paquier
Date:
On Fri, Dec 20, 2019 at 01:08:47AM +0000, 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.yzbfzuppjritidqr@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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Michael Paquier
Date:
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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Kyotaro Horiguchi
Date:
At Fri, 31 Jan 2020 11:47:01 +0900, Michael Paquier <michael@paquier.xyz> 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 exposeinconsistencies.

From
Michael Paquier
Date:
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

Attachment

Re: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
Michael Paquier
Date:
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

Attachment

RE: Proposal: Add more compile-time asserts to exposeinconsistencies.

From
"Smith, Peter"
Date:
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 exposeinconsistencies.

From
Michael Paquier
Date:
On Tue, Mar 10, 2020 at 12:22:32AM +0000, 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

Attachment