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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: "Smith, Peter" <peters(at)fast(dot)au(dot)fujitsu(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "peter(dot)eisentraut(at)2ndquadrant(dot)com" <peter(dot)eisentraut(at)2ndquadrant(dot)com>, "tgl(at)sss(dot)pgh(dot)pa(dot)us" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "ilmari(at)ilmari(dot)org" <ilmari(at)ilmari(dot)org>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Proposal: Add more compile-time asserts to expose inconsistencies.
Date: 2019-12-04 06:16:25
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro Horiguchi 2019-12-04 06:32:11 Re: Increase footprint of %m and reduce strerror()
Previous Message Amit Langote 2019-12-04 06:10:55 Re: pgbench -i progress output on terminal