Thread: Refactor compile-time assertion checks for C/C++
Hi all, As of [1], I have been playing with the compile time assertions that we have for expressions, declarations and statements. And it happens that it is visibly possible to consolidate the fallback implementations for C and C++. Attached is the result of what I am getting at. I am adding this patch to next CF. Thoughts are welcome. [1]: https://www.postgresql.org/message-id/201DD0641B056142AC8C6645EC1B5F62014B8E8030@SYD1217 Thanks, -- Michael
Attachment
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation: not tested In my humble opinion the patch improves readability, hence gets my +1. No further suggestions. Passing on to a committer to judge further. The new status of this patch is: Ready for Committer
Michael Paquier <michael@paquier.xyz> writes:
> As of [1], I have been playing with the compile time assertions that
> we have for expressions, declarations and statements. And it happens
> that it is visibly possible to consolidate the fallback
> implementations for C and C++. Attached is the result of what I am
> getting at. I am adding this patch to next CF. Thoughts are
> welcome.
cfbot reports this doesn't work with MSVC. Not sure why --- maybe
it defines __cpp_static_assert differently than you're expecting?
regards, tom lane
On Sat, Mar 07, 2020 at 04:44:48PM -0500, Tom Lane wrote: > cfbot reports this doesn't work with MSVC. Not sure why --- maybe > it defines __cpp_static_assert differently than you're expecting? I don't think that's the issue. The CF bot uses MSVC 12.0 which refers to the 2013. __cpp_static_assert being introduced in MSVC 2017, this error is visibly telling us that this environment does not like the C++ fallback implementation, which is actually what my previous version of the patch was using (I can reproduce the error with my MSVC 2015 VM as well). I think that this points to an error in the patch: for the refactoring, the fallback implementation of C and C++ should use the fallback implementation for C that we have currently on HEAD. With the updated patch attached, the error goes away for me. Let's see what Mr. Robot thinks. The patch was marked as ready for committer, I am switching it back to "Needs review". -- Michael
Attachment
Thank you for updating the status of the issue. I have to admit that I completely missed the CF bot. Lesson learned. For whatever is worth, my previous comment that the patch improves readability also applies to the updated version of the patch. The CF bot seems happy now, which means that your assessment as to the error and fix are correct. So :+1: from me.
On Wed, Mar 11, 2020 at 12:31:19PM +0000, Georgios Kokolatos wrote: > For whatever is worth, my previous comment that the patch improves > readability also applies to the updated version of the patch. v2 has actually less diffs for the C++ part. > The CF bot seems happy now, which means that your assessment as > to the error and fix are correct. Indeed the bot is happy now. By looking at the patch, one would note that what it just does is unifying the fallback "hack-ish" implementations so as C and C++ use the same thing, which is the fallback implementation for C of HEAD. I would prefer hear first from more people to see if they like this change. Or not. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> Indeed the bot is happy now. By looking at the patch, one would note
> that what it just does is unifying the fallback "hack-ish"
> implementations so as C and C++ use the same thing, which is the
> fallback implementation for C of HEAD. I would prefer hear first from
> more people to see if they like this change. Or not.
I looked at this and tried it on an old (non HAVE__STATIC_ASSERT)
gcc version. Seems to work, but I have a couple cosmetic suggestions:
1. The comment block above this was never updated to mention that
we're now catering for C++ too. Maybe something like
* gcc 4.6 and up supports _Static_assert(), but there are bizarre syntactic
* placement restrictions. Macros StaticAssertStmt() and StaticAssertExpr()
* make it safe to use as a statement or in an expression, respectively.
* The macro StaticAssertDecl() is suitable for use at file scope (outside of
* any function).
*
+ * On recent C++ compilers, we can use standard static_assert().
+ *
* 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.
2. I think you could simplify the #elif to just
#elif defined(__cplusplus) && __cpp_static_assert >= 200410
Per the C standard, an unrecognized identifier in an #if condition
is replaced by zero. So the condition will come out false as desired
if __cpp_static_assert isn't defined; you don't need to test that
separately.
regards, tom lane
On Thu, Mar 12, 2020 at 12:33:21AM -0400, Tom Lane wrote: > I looked at this and tried it on an old (non HAVE__STATIC_ASSERT) > gcc version. Seems to work, but I have a couple cosmetic suggestions: Thanks for the review. > 1. The comment block above this was never updated to mention that > we're now catering for C++ too. Maybe something like > > + * On recent C++ compilers, we can use standard static_assert(). > + * Sounds fine to me. Looking here this is present since GCC 4.3: https://gcc.gnu.org/projects/cxx-status.html#cxx11 For MSVC, actually I was a bit wrong, only the flavor without error message is supported since VS 2017, and the one we use is much older: https://docs.microsoft.com/en-us/cpp/cpp/static-assert?view=vs-2015 So, should we add a reference about both in the new comment? I would actually not add them, so I have used your suggestion in the attached, but the comment block above does that for _Static_assert(). Do you think it is better to add some references to some of those compilers (say GCC 4.3, MSVC)? Just stick with your suggestion? Or stick with your version and replace the reference to GCC 4.6 with something like "recent compilers"? > 2. I think you could simplify the #elif to just > > #elif defined(__cplusplus) && __cpp_static_assert >= 200410 > > Per the C standard, an unrecognized identifier in an #if condition > is replaced by zero. So the condition will come out false as desired > if __cpp_static_assert isn't defined; you don't need to test that > separately. Thanks, indeed. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> So, should we add a reference about both in the new comment? I would
> actually not add them, so I have used your suggestion in the attached,
> but the comment block above does that for _Static_assert(). Do you
> think it is better to add some references to some of those compilers
> (say GCC 4.3, MSVC)? Just stick with your suggestion? Or stick with
> your version and replace the reference to GCC 4.6 with something like
> "recent compilers"?
I don't feel a need to expend a whole lot of sweat there. The existing
text is fine, it just bugged me that the code deals with three cases
while the comment block only acknowledged two. So I'd just go with
what you have in v3.
regards, tom lane
On Thu, Mar 12, 2020 at 09:43:54AM -0400, Tom Lane wrote: > I don't feel a need to expend a whole lot of sweat there. The existing > text is fine, it just bugged me that the code deals with three cases > while the comment block only acknowledged two. So I'd just go with > what you have in v3. Thanks, Tom. I have committed v3 then. -- Michael
Attachment
On Fri, Mar 13, 2020 at 03:12:34PM +0900, Michael Paquier wrote: > On Thu, Mar 12, 2020 at 09:43:54AM -0400, Tom Lane wrote: >> I don't feel a need to expend a whole lot of sweat there. The existing >> text is fine, it just bugged me that the code deals with three cases >> while the comment block only acknowledged two. So I'd just go with >> what you have in v3. > > Thanks, Tom. I have committed v3 then. Hmm. v3 actually broke the C++ fallback of StaticAssertExpr() and StaticAssertStmt() (v1 did not), a simple fix being something like the attached. The buildfarm does not really care about that, but it could for example by using the only c++ code compiled in the tree in src/backend/jit/? That also means that only builds using --with-llvm with a compiler old enough would trigger that stuff. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> Hmm. v3 actually broke the C++ fallback of StaticAssertExpr() and
> StaticAssertStmt() (v1 did not), a simple fix being something like
> the attached.
The buildfarm seems happy, so why do you think it's broken?
If we do need to change it, I'd be inclined to just use the do{}
block everywhere, not bothering with the extra #if test.
regards, tom lane
On Fri, Mar 13, 2020 at 11:00:33AM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> Hmm. v3 actually broke the C++ fallback of StaticAssertExpr() and
>> StaticAssertStmt() (v1 did not), a simple fix being something like
>> the attached.
>
> The buildfarm seems happy, so why do you think it's broken?
Extensions like the attached don't appreciate it, and we have nothing
like that in core. Perhaps we could, but it is not really appealing
for all platforms willing to run the tests to require CXX or such..
> If we do need to change it, I'd be inclined to just use the do{}
> block everywhere, not bothering with the extra #if test.
Not sure what you mean here because we cannot use the do{} flavor
either for the C fallback, no? See for example the definitions of
unconstify() in c.h.
--
Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Mar 13, 2020 at 11:00:33AM -0400, Tom Lane wrote:
>> If we do need to change it, I'd be inclined to just use the do{}
>> block everywhere, not bothering with the extra #if test.
> Not sure what you mean here because we cannot use the do{} flavor
> either for the C fallback, no? See for example the definitions of
> unconstify() in c.h.
Sorry for being unclear --- I just meant that we could use do{}
in StaticAssertStmt for both C and C++. Although now I notice
that the code is trying to use StaticAssertStmt for StaticAssertExpr,
which you're right isn't going to do. But I think something like
this would work and be a bit simpler than what you proposed:
#else
/* Fallback implementation for C and C++ */
#define StaticAssertStmt(condition, errmessage) \
- ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
+ do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0)
#define StaticAssertExpr(condition, errmessage) \
- StaticAssertStmt(condition, errmessage)
+ ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
#define StaticAssertDecl(condition, errmessage) \
regards, tom lane
On Mon, Mar 16, 2020 at 10:32:36AM -0400, Tom Lane wrote:
> Sorry for being unclear --- I just meant that we could use do{}
> in StaticAssertStmt for both C and C++. Although now I notice
> that the code is trying to use StaticAssertStmt for StaticAssertExpr,
> which you're right isn't going to do. But I think something like
> this would work and be a bit simpler than what you proposed:
>
> #else
> /* Fallback implementation for C and C++ */
> #define StaticAssertStmt(condition, errmessage) \
> - ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
> + do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0)
> #define StaticAssertExpr(condition, errmessage) \
> - StaticAssertStmt(condition, errmessage)
> + ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
> #define StaticAssertDecl(condition, errmessage) \
C++ does not allow defining a struct inside a sizeof() call, so in
this case StaticAssertExpr() does not work with the previous extension
in C++. StaticAssertStmt() does the work though.
One alternatine I can think of for C++ would be something like the
following, though C does not like this flavor either:
typedef char static_assert_struct[condition ? 1 : -1]
--
Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Mar 16, 2020 at 10:32:36AM -0400, Tom Lane wrote:
>> Sorry for being unclear --- I just meant that we could use do{}
>> in StaticAssertStmt for both C and C++. Although now I notice
>> that the code is trying to use StaticAssertStmt for StaticAssertExpr,
>> which you're right isn't going to do. But I think something like
>> this would work and be a bit simpler than what you proposed:
>>
>> #else
>> /* Fallback implementation for C and C++ */
>> #define StaticAssertStmt(condition, errmessage) \
>> - ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
>> + do { struct static_assert_struct { int static_assert_failure : (condition) ? 1 : -1; }; } while(0)
>> #define StaticAssertExpr(condition, errmessage) \
>> - StaticAssertStmt(condition, errmessage)
>> + ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
>> #define StaticAssertDecl(condition, errmessage) \
> C++ does not allow defining a struct inside a sizeof() call, so in
> this case StaticAssertExpr() does not work with the previous extension
> in C++. StaticAssertStmt() does the work though.
[ scratches head... ] A do{} is okay in an expression in C++ ??
regards, tom lane
On Mon, Mar 16, 2020 at 10:35:05PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> C++ does not allow defining a struct inside a sizeof() call, so in
>> this case StaticAssertExpr() does not work with the previous extension
>> in C++. StaticAssertStmt() does the work though.
>
> [ scratches head... ] A do{} is okay in an expression in C++ ??
cpp-fallback-fix.patch in [1] was doing that.
The fun does not stop here. gcc is fine when using that for C and
C++:
#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); }))
But then problems come from MSVC which does not like the do{} part for
statements, and this works:
#define StaticAssertStmt(condition, errmessage) \
((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
#define StaticAssertExpr(condition, errmessage) \
StaticAssertStmt(condition, errmessage)
[1]: https://postgr.es/m/20200313115033.GA183471@paquier.xyz
--
Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> The fun does not stop here. gcc is fine when using that for C and
> C++:
> #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); }))
Hm, I'm not so sure. I just noticed that cpluspluscheck is failing
for me now:
$ src/tools/pginclude/cpluspluscheck
In file included from /tmp/cpluspluscheck.HRgpVA/test.cpp:4:
./src/include/common/int128.h: In function 'void int128_add_int64_mul_int64(INT128*, int64, int64)':
./src/include/common/int128.h:180: error: types may not be defined in 'sizeof' expressions
which of course is pointing at
StaticAssertStmt(((int64) -1 >> 1) == (int64) -1,
"arithmetic right shift is needed");
so the existing "C and C++" fallback StaticAssertStmt doesn't work for
older g++. (This is g++ 4.4.7 from RHEL6.)
> But then problems come from MSVC which does not like the do{} part for
> statements, and this works:
Huh? Surely do{} is a legal statement.
Maybe we should just revert b7f64c64d instead of putting more time
into this. It's looking like we're going to end up with four or so
implementations no matter what, so it's getting hard to see any
real benefit.
regards, tom lane
On Sat, Mar 21, 2020 at 07:22:41PM -0400, Tom Lane wrote:
> which of course is pointing at
>
> StaticAssertStmt(((int64) -1 >> 1) == (int64) -1,
> "arithmetic right shift is needed");
>
> so the existing "C and C++" fallback StaticAssertStmt doesn't work for
> older g++. (This is g++ 4.4.7 from RHEL6.)
Hmm. Thanks. I just have an access down to 7.5 on my machine.
> Huh? Surely do{} is a legal statement.
Yep, still my VS-2015 compiler complains when using the fallback with
do{} for statements, and I am not sure why. An extra choice coming to
my mind would be to use a more native MS implementation, as documented
here:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/assert-asserte-assert-expr-macros?view=vs-2019
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/static-assert-macro?view=vs-2019
This requires an extra branch in our implementation set which is not
really appealing, with I guess the following mapping (not tested):
- _STATIC_ASSERT for StaticAssertDecl and StaticAssertExpr.
- _ASSERT_EXPR for and StaticAssertStmt.
I think that one advantage is that this would allow to simplify the
fallback implementations for C/C++ to use do{}s.
> Maybe we should just revert b7f64c64d instead of putting more time
> into this. It's looking like we're going to end up with four or so
> implementations no matter what, so it's getting hard to see any
> real benefit.
Indeed. I have tried a couple of other things I could think of, but I
cannot really get down to 3 implementations, so there is no actual
benefit.
I have done a complete revert to keep the history cleaner for release
notes and such, including this part:
- * On recent C++ compilers, we can use standard static_assert().
Don't you think that we should keep this comment at the end? It is
still true.
Thanks for the discussion!
--
Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Sat, Mar 21, 2020 at 07:22:41PM -0400, Tom Lane wrote:
>> Maybe we should just revert b7f64c64d instead of putting more time
>> into this. It's looking like we're going to end up with four or so
>> implementations no matter what, so it's getting hard to see any
>> real benefit.
> Indeed. I have tried a couple of other things I could think of, but I
> cannot really get down to 3 implementations, so there is no actual
> benefit.
> I have done a complete revert to keep the history cleaner for release
> notes and such, including this part:
> - * On recent C++ compilers, we can use standard static_assert().
> Don't you think that we should keep this comment at the end? It is
> still true.
Yeah, the comment needs an update; but if we have four implementations
then it ought to describe each of them, IMO.
regards, tom lane
On Mon, Mar 23, 2020 at 12:22:48AM -0400, Tom Lane wrote: > Yeah, the comment needs an update; but if we have four implementations > then it ought to describe each of them, IMO. I got an idea as per the attached. Perhaps you have a better idea? -- Michael