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

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