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

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