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
From 3872af56ad51ba2f6d927ffd6e7f79a0bd08551a Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 31 Jan 2020 10:34:30 +0900
Subject: [PATCH 1/2] Add declaration-level assertions
---
src/include/c.h | 24 ++++++++++++------
src/include/storage/bufpage.h | 10 ++++++++
src/backend/storage/page/bufpage.c | 9 +------
src/backend/utils/adt/lockfuncs.c | 6 +++++
src/backend/utils/misc/guc.c | 39 ++++++++++++++++++++++++++++++
src/common/relpath.c | 3 +++
src/bin/pg_dump/pg_dump_sort.c | 3 +++
7 files changed, 79 insertions(+), 15 deletions(-)
diff --git a/src/include/c.h b/src/include/c.h
index 6898229b43..b2966f9edf 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -826,14 +826,16 @@ extern void ExceptionalCondition(const char *conditionName,
#endif
/*
- * Macros to support compile-time assertion checks.
+ * Macros to support compile-time and declaration assertion checks.
*
- * 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).
*
* gcc 4.6 and up supports _Static_assert(), but there are bizarre syntactic
- * placement restrictions. These macros make it safe to use as a statement
- * or in an expression, respectively.
+ * placement restrictions. Compile-time macros make it safe to use as a
+ * statement or in an expression, respectively. Declaration macros are
+ * useful at file scope.
*
* 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
@@ -845,11 +847,15 @@ extern void ExceptionalCondition(const char *conditionName,
do { _Static_assert(condition, errmessage); } while(0)
#define StaticAssertExpr(condition, errmessage) \
((void) ({ StaticAssertStmt(condition, errmessage); true; }))
+#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
@@ -857,12 +863,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++ */
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 870ecb51b7..3f88683a05 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -418,6 +418,16 @@ do { \
((overwrite) ? PAI_OVERWRITE : 0) | \
((is_heap) ? PAI_IS_HEAP : 0))
+/*
+ * Check that BLCKSZ is a multiple of sizeof(size_t). In PageIsVerified(),
+ * it is much faster to check if a page is full of zeroes using the native
+ * word size. Note that this assertion is kept within a header to make
+ * sure that StaticAssertDecl() works across various combinations of
+ * platforms and compilers.
+ */
+StaticAssertDecl(BLCKSZ == ((BLCKSZ / sizeof(size_t)) * sizeof(size_t)),
+ "BLCKSZ has to be a multiple of sizeof(size_t)");
+
extern void PageInit(Page page, Size pageSize, Size specialSize);
extern bool PageIsVerified(Page page, BlockNumber blkno);
extern OffsetNumber PageAddItemExtended(Page page, Item item, Size size,
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index f47176753d..4ea6ea7a3d 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -119,14 +119,7 @@ PageIsVerified(Page page, BlockNumber blkno)
return true;
}
- /*
- * Check all-zeroes case. Luckily BLCKSZ is guaranteed to always be a
- * multiple of size_t - and it's much faster to compare memory using the
- * native word size.
- */
- StaticAssertStmt(BLCKSZ == (BLCKSZ / sizeof(size_t)) * sizeof(size_t),
- "BLCKSZ has to be a multiple of sizeof(size_t)");
-
+ /* Check all-zeroes case */
all_zeroes = true;
pagebytes = (size_t *) page;
for (i = 0; i < (BLCKSZ / sizeof(size_t)); i++)
diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
index 9a7da12a5b..7e47ebeb6f 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -36,6 +36,9 @@ const char *const LockTagTypeNames[] = {
"advisory"
};
+StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1),
+ "array length mismatch");
+
/* This must match enum PredicateLockTargetType (predicate_internals.h) */
static const char *const PredicateLockTagTypeNames[] = {
"relation",
@@ -43,6 +46,9 @@ static const char *const PredicateLockTagTypeNames[] = {
"tuple"
};
+StaticAssertDecl(lengthof(PredicateLockTagTypeNames) == (PREDLOCKTAG_TUPLE + 1),
+ "array length mismatch");
+
/* Working status for pg_lock_status */
typedef struct
{
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a16fe8cd5b..9630866a5f 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -241,6 +241,9 @@ static const struct config_enum_entry bytea_output_options[] = {
{NULL, 0, false}
};
+StaticAssertDecl(lengthof(bytea_output_options) == (BYTEA_OUTPUT_HEX + 2),
+ "array length mismatch");
+
/*
* We have different sets for client and server message level options because
* they sort slightly different (see "log" level), and because "fatal"/"panic"
@@ -286,6 +289,9 @@ static const struct config_enum_entry intervalstyle_options[] = {
{NULL, 0, false}
};
+StaticAssertDecl(lengthof(intervalstyle_options) == (INTSTYLE_ISO_8601 + 2),
+ "array length mismatch");
+
static const struct config_enum_entry log_error_verbosity_options[] = {
{"terse", PGERROR_TERSE, false},
{"default", PGERROR_DEFAULT, false},
@@ -293,6 +299,9 @@ static const struct config_enum_entry log_error_verbosity_options[] = {
{NULL, 0, false}
};
+StaticAssertDecl(lengthof(log_error_verbosity_options) == (PGERROR_VERBOSE + 2),
+ "array length mismatch");
+
static const struct config_enum_entry log_statement_options[] = {
{"none", LOGSTMT_NONE, false},
{"ddl", LOGSTMT_DDL, false},
@@ -301,6 +310,9 @@ static const struct config_enum_entry log_statement_options[] = {
{NULL, 0, false}
};
+StaticAssertDecl(lengthof(log_statement_options) == (LOGSTMT_ALL + 2),
+ "array length mismatch");
+
static const struct config_enum_entry isolation_level_options[] = {
{"serializable", XACT_SERIALIZABLE, false},
{"repeatable read", XACT_REPEATABLE_READ, false},
@@ -316,6 +328,9 @@ static const struct config_enum_entry session_replication_role_options[] = {
{NULL, 0, false}
};
+StaticAssertDecl(lengthof(session_replication_role_options) == (SESSION_REPLICATION_ROLE_LOCAL + 2),
+ "array length mismatch");
+
static const struct config_enum_entry syslog_facility_options[] = {
#ifdef HAVE_SYSLOG
{"local0", LOG_LOCAL0, false},
@@ -339,18 +354,27 @@ static const struct config_enum_entry track_function_options[] = {
{NULL, 0, false}
};
+StaticAssertDecl(lengthof(track_function_options) == (TRACK_FUNC_ALL + 2),
+ "array length mismatch");
+
static const struct config_enum_entry xmlbinary_options[] = {
{"base64", XMLBINARY_BASE64, false},
{"hex", XMLBINARY_HEX, false},
{NULL, 0, false}
};
+StaticAssertDecl(lengthof(xmlbinary_options) == (XMLBINARY_HEX + 2),
+ "array length mismatch");
+
static const struct config_enum_entry xmloption_options[] = {
{"content", XMLOPTION_CONTENT, false},
{"document", XMLOPTION_DOCUMENT, false},
{NULL, 0, false}
};
+StaticAssertDecl(lengthof(xmloption_options) == (XMLOPTION_CONTENT + 2),
+ "array length mismatch");
+
/*
* Although only "on", "off", and "safe_encoding" are documented, we
* accept all the likely variants of "on" and "off".
@@ -465,6 +489,9 @@ const struct config_enum_entry ssl_protocol_versions_info[] = {
{NULL, 0, false}
};
+StaticAssertDecl(lengthof(ssl_protocol_versions_info) == (PG_TLS1_3_VERSION + 2),
+ "array length mismatch");
+
static struct config_enum_entry shared_memory_options[] = {
#ifndef WIN32
{"sysv", SHMEM_TYPE_SYSV, false},
@@ -615,6 +642,9 @@ const char *const GucContext_Names[] =
/* PGC_USERSET */ "user"
};
+StaticAssertDecl(lengthof(GucContext_Names) == (PGC_USERSET + 1),
+ "array length mismatch");
+
/*
* Displayable names for source types (enum GucSource)
*
@@ -638,6 +668,9 @@ const char *const GucSource_Names[] =
/* PGC_S_SESSION */ "session"
};
+StaticAssertDecl(lengthof(GucSource_Names) == (PGC_S_SESSION + 1),
+ "array length mismatch");
+
/*
* Displayable names for the groupings defined in enum config_group
*/
@@ -749,6 +782,9 @@ const char *const config_group_names[] =
NULL
};
+StaticAssertDecl(lengthof(config_group_names) == (DEVELOPER_OPTIONS + 2),
+ "array length mismatch");
+
/*
* Displayable names for GUC variable types (enum config_type)
*
@@ -763,6 +799,9 @@ const char *const config_type_names[] =
/* PGC_ENUM */ "enum"
};
+StaticAssertDecl(lengthof(config_type_names) == (PGC_ENUM + 1),
+ "array length mismatch");
+
/*
* Unit conversion tables.
*
diff --git a/src/common/relpath.c b/src/common/relpath.c
index 91efcedf70..ad733d1363 100644
--- a/src/common/relpath.c
+++ b/src/common/relpath.c
@@ -37,6 +37,9 @@ const char *const forkNames[] = {
"init" /* INIT_FORKNUM */
};
+StaticAssertDecl(lengthof(forkNames) == (MAX_FORKNUM + 1),
+ "array length mismatch");
+
/*
* forkname_to_number - look up fork number by name
*
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 464798cf5c..1432dd7e0d 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -80,6 +80,9 @@ static const int dbObjectTypePriority[] =
38 /* DO_SUBSCRIPTION */
};
+StaticAssertDecl(lengthof(dbObjectTypePriority) == (DO_SUBSCRIPTION + 1),
+ "array length mismatch");
+
static DumpId preDataBoundId;
static DumpId postDataBoundId;
--
2.25.0
From d31bfc1b70e3dca83ce4d6d73776d519790359bd Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 31 Jan 2020 11:37:10 +0900
Subject: [PATCH 2/2] Refactor assertion definitions in c.h
This unifies the C and C++ fallback implementations.
---
src/include/c.h | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/src/include/c.h b/src/include/c.h
index b2966f9edf..22d77e8d0c 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -841,39 +841,32 @@ extern void ExceptionalCondition(const char *conditionName,
* 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.
*/
-#ifndef __cplusplus
-#ifdef HAVE__STATIC_ASSERT
+#if !defined(__cplusplus) && defined(HAVE__STATIC_ASSERT)
+/* Default C implementation */
#define StaticAssertStmt(condition, errmessage) \
do { _Static_assert(condition, errmessage); } while(0)
#define StaticAssertExpr(condition, errmessage) \
((void) ({ StaticAssertStmt(condition, errmessage); true; }))
#define StaticAssertDecl(condition, errmessage) \
_Static_assert(condition, errmessage)
-#else /* !HAVE__STATIC_ASSERT */
+#elif defined(__cplusplus) && \
+ defined(__cpp_static_assert) && __cpp_static_assert >= 200410
+/* Default C++ implementation */
#define StaticAssertStmt(condition, errmessage) \
- ((void) sizeof(struct { int static_assert_failure : (condition) ? 1 : -1; }))
+ static_assert(condition, errmessage)
#define StaticAssertExpr(condition, errmessage) \
+ ({ StaticAssertStmt(condition, errmessage); })
+#define StaticAssertDecl(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
-#define StaticAssertStmt(condition, errmessage) \
- static_assert(condition, errmessage)
-#define StaticAssertExpr(condition, errmessage) \
- ({ static_assert(condition, errmessage); })
-#define StaticAssertDecl(condition, errmessage) \
- static_assert(condition, errmessage)
-#else /* !__cpp_static_assert */
+#else
+/* Fallback implementation 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); }))
#define StaticAssertDecl(condition, errmessage) \
extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1])
-#endif /* __cpp_static_assert */
-#endif /* C++ */
+#endif
/*
--
2.25.0
signature.asc
Description: PGP signature
