On 15.11.25 01:06, Thomas Munro wrote:
Our assertion quality is lower on Visual Studio. I assumed there was nothing stopping us from writing a pg_expr_has_type_p() macro using pure standard C and C++ these days, and the attached patch seemed to work on GCC, Clang and Visual Studio 2022 (via CI, no Windows here).Unfortunately our unconstify() macro triggers internal errors on Visual Studio 2019 with this applied: [23:39:47.176] ../src/common/file_utils.c(712): fatal error C1001: Internal compiler error. [23:39:47.176] (compiler file 'msc1.cpp', line 1603) [23:39:47.176] To work around this problem, try simplifying or changing the program near the locations listed above. Presumably _Generic type resolution and StaticAssertExpr()'s definition are just too much for it. I wonder if some other phrasing could help. Posting where I got to with this, in case anyone has any ideas...
Yeah, I had been playing with a similar patch, which now also crashes on CI, but I'm pretty sure this worked at some point. So maybe an upgrade or downgrade would fix it.
Note however that similar to the comment I made over about stdatomic.h, we currently still have buildfarm members with gcc <4.9, which don't support _Generic.
The attached 0001 patch, which is also contained in your patch, should be applied nonetheless. After researching the history, I think when relptr.h was added, it just took a shortcut with the configure checks available at the time, and we should just correct it now.
I did not consider C++. I'm unsure what to do about it. For the C type system, "compatible" is term of art, and swapping __builtin_types_compatible_p for _Generic is semantically equivalent. I don't have the C++ experience to know what exactly std::is_same is, but I don't know that we want to expose ourselves to requiring types to be *both* C-"compatible" and C++-"same" without more guidance.
In your patch, you also rewrite the unconstify() and unvolatize() macros. At least in my head, these were initially modeled as C versions of C++ const_cast. So it seems morally imperative to keep that association. ;-)
I don't know what the aim of the C++ support might be. Is there a chance that with the right set of changes, for example, "lib/ilist.h" or "lib/pairingheap.h" could be used under C++?
From 26d2cbd13b1c9057ad22f8b02e34816b3338c4db Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Wed, 19 Nov 2025 19:02:46 +0100 Subject: [PATCH vPE 1/2] Use correct preprocessor conditional in relptr.h When relptr.h was added (commit fbc1c12a94a), there was no check for HAVE_TYPEOF, so it used HAVE__BUILTIN_TYPES_COMPATIBLE_P, which already existed (commit ea473fb2dee) and which was thought to cover approximately the same compilers. But the guarded code can also work without HAVE__BUILTIN_TYPES_COMPATIBLE_P, and we now have a check for HAVE_TYPEOF (commit 4cb824699e1), so let's fix this up to use the correct logic. --- src/include/utils/relptr.h | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/include/utils/relptr.h b/src/include/utils/relptr.h index ea340fee657..48e394dba71 100644 --- a/src/include/utils/relptr.h +++ b/src/include/utils/relptr.h @@ -38,16 +38,12 @@ #define relptr_declare(type, relptrtype) \ typedef relptr(type) relptrtype -#ifdef HAVE__BUILTIN_TYPES_COMPATIBLE_P +#ifdef HAVE_TYPEOF #define relptr_access(base, rp) \ (AssertVariableIsOfTypeMacro(base, char *), \ - (__typeof__((rp).relptr_type)) ((rp).relptr_off == 0 ? NULL : \ + (typeof((rp).relptr_type)) ((rp).relptr_off == 0 ? NULL : \ (base) + (rp).relptr_off - 1)) #else -/* - * If we don't have __builtin_types_compatible_p, assume we might not have - * __typeof__ either. - */ #define relptr_access(base, rp) \ (AssertVariableIsOfTypeMacro(base, char *), \ (void *) ((rp).relptr_off == 0 ? NULL : (base) + (rp).relptr_off - 1)) @@ -72,16 +68,12 @@ relptr_store_eval(char *base, char *val) } } -#ifdef HAVE__BUILTIN_TYPES_COMPATIBLE_P +#ifdef HAVE_TYPEOF #define relptr_store(base, rp, val) \ (AssertVariableIsOfTypeMacro(base, char *), \ - AssertVariableIsOfTypeMacro(val, __typeof__((rp).relptr_type)), \ + AssertVariableIsOfTypeMacro(val, typeof((rp).relptr_type)), \ (rp).relptr_off = relptr_store_eval((base), (char *) (val))) #else -/* - * If we don't have __builtin_types_compatible_p, assume we might not have - * __typeof__ either. - */ #define relptr_store(base, rp, val) \ (AssertVariableIsOfTypeMacro(base, char *), \ (rp).relptr_off = relptr_store_eval((base), (char *) (val))) -- 2.51.0
From 941d34cf597bf332e8b404a48c6e61b6e4e07f1a Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Wed, 19 Nov 2025 19:24:00 +0100 Subject: [PATCH vPE 2/2] Replace __builtin_types_compatible_p with _Generic This is the C11 standard equivalent, so we can now rely on this working on all compilers. --- config/c-compiler.m4 | 19 ------------------- configure | 30 ------------------------------ configure.ac | 1 - meson.build | 15 --------------- src/include/c.h | 33 ++++++--------------------------- src/include/pg_config.h.in | 3 --- 6 files changed, 6 insertions(+), 95 deletions(-) diff --git a/config/c-compiler.m4 b/config/c-compiler.m4 index 236a59e8536..26d5269e58a 100644 --- a/config/c-compiler.m4 +++ b/config/c-compiler.m4 @@ -161,25 +161,6 @@ fi])# PGAC_C_TYPEOF -# PGAC_C_TYPES_COMPATIBLE -# ----------------------- -# Check if the C compiler understands __builtin_types_compatible_p, -# and define HAVE__BUILTIN_TYPES_COMPATIBLE_P if so. -# -# We check usage with __typeof__, though it's unlikely any compiler would -# have the former and not the latter. -AC_DEFUN([PGAC_C_TYPES_COMPATIBLE], -[AC_CACHE_CHECK(for __builtin_types_compatible_p, pgac_cv__types_compatible, -[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], -[[ int x; static int y[__builtin_types_compatible_p(__typeof__(x), int)]; ]])], -[pgac_cv__types_compatible=yes], -[pgac_cv__types_compatible=no])]) -if test x"$pgac_cv__types_compatible" = xyes ; then -AC_DEFINE(HAVE__BUILTIN_TYPES_COMPATIBLE_P, 1, - [Define to 1 if your compiler understands __builtin_types_compatible_p.]) -fi])# PGAC_C_TYPES_COMPATIBLE - - # PGAC_C_BUILTIN_CONSTANT_P # ------------------------- # Check if the C compiler understands __builtin_constant_p(), diff --git a/configure b/configure index 3a0ed11fa8e..5ad57aff950 100755 --- a/configure +++ b/configure @@ -14786,36 +14786,6 @@ cat >>confdefs.h <<_ACEOF _ACEOF fi -fi -{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for __builtin_types_compatible_p" >&5 -$as_echo_n "checking for __builtin_types_compatible_p... " >&6; } -if ${pgac_cv__types_compatible+:} false; then : - $as_echo_n "(cached) " >&6 -else - cat confdefs.h - <<_ACEOF >conftest.$ac_ext -/* end confdefs.h. */ - -int -main () -{ - int x; static int y[__builtin_types_compatible_p(__typeof__(x), int)]; - ; - return 0; -} -_ACEOF -if ac_fn_c_try_compile "$LINENO"; then : - pgac_cv__types_compatible=yes -else - pgac_cv__types_compatible=no -fi -rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext -fi -{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv__types_compatible" >&5 -$as_echo "$pgac_cv__types_compatible" >&6; } -if test x"$pgac_cv__types_compatible" = xyes ; then - -$as_echo "#define HAVE__BUILTIN_TYPES_COMPATIBLE_P 1" >>confdefs.h - fi { $as_echo "$as_me:${as_lineno-$LINENO}: checking for __builtin_constant_p" >&5 $as_echo_n "checking for __builtin_constant_p... " >&6; } diff --git a/configure.ac b/configure.ac index c2413720a18..e2821b665c1 100644 --- a/configure.ac +++ b/configure.ac @@ -1676,7 +1676,6 @@ AC_C_INLINE PGAC_PRINTF_ARCHETYPE PGAC_C_STATIC_ASSERT PGAC_C_TYPEOF -PGAC_C_TYPES_COMPATIBLE PGAC_C_BUILTIN_CONSTANT_P PGAC_C_BUILTIN_OP_OVERFLOW PGAC_C_BUILTIN_UNREACHABLE diff --git a/meson.build b/meson.build index c1e17aa3040..5170e0748f5 100644 --- a/meson.build +++ b/meson.build @@ -1949,21 +1949,6 @@ foreach builtin : builtins endforeach -# Check if the C compiler understands __builtin_types_compatible_p, -# and define HAVE__BUILTIN_TYPES_COMPATIBLE_P if so. -# -# We check usage with __typeof__, though it's unlikely any compiler would -# have the former and not the latter. -if cc.compiles(''' - static int x; - static int y[__builtin_types_compatible_p(__typeof__(x), int)]; - ''', - name: '__builtin_types_compatible_p', - args: test_c_args) - cdata.set('HAVE__BUILTIN_TYPES_COMPATIBLE_P', 1) -endif - - # Check if the C compiler understands __builtin_$op_overflow(), # and define HAVE__BUILTIN_OP_OVERFLOW if so. # diff --git a/src/include/c.h b/src/include/c.h index cb8a38669be..f105475e425 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -968,26 +968,13 @@ pg_noreturn extern void ExceptionalCondition(const char *conditionName, * AssertVariableIsOfType() can be used as a statement. * AssertVariableIsOfTypeMacro() is intended for use in macros, eg * #define foo(x) (AssertVariableIsOfTypeMacro(x, int), bar(x)) - * - * If we don't have __builtin_types_compatible_p, we can still assert that - * the types have the same size. This is far from ideal (especially on 32-bit - * platforms) but it provides at least some coverage. */ -#ifdef HAVE__BUILTIN_TYPES_COMPATIBLE_P -#define AssertVariableIsOfType(varname, typename) \ - StaticAssertStmt(__builtin_types_compatible_p(__typeof__(varname), typename), \ - CppAsString(varname) " does not have type " CppAsString(typename)) -#define AssertVariableIsOfTypeMacro(varname, typename) \ - (StaticAssertExpr(__builtin_types_compatible_p(__typeof__(varname), typename), \ - CppAsString(varname) " does not have type " CppAsString(typename))) -#else /* !HAVE__BUILTIN_TYPES_COMPATIBLE_P */ #define AssertVariableIsOfType(varname, typename) \ - StaticAssertStmt(sizeof(varname) == sizeof(typename), \ + StaticAssertStmt(_Generic((varname), typename: 1, default: 0), \ CppAsString(varname) " does not have type " CppAsString(typename)) #define AssertVariableIsOfTypeMacro(varname, typename) \ - (StaticAssertExpr(sizeof(varname) == sizeof(typename), \ + (StaticAssertExpr(_Generic((varname), typename: 1, default: 0), \ CppAsString(varname) " does not have type " CppAsString(typename))) -#endif /* HAVE__BUILTIN_TYPES_COMPATIBLE_P */ /* ---------------------------------------------------------------- @@ -1222,8 +1209,7 @@ typedef union PGAlignedXLogBlock /* * Macro that allows to cast constness and volatile away from an expression, but doesn't - * allow changing the underlying type. Enforcement of the latter - * currently only works for gcc like compilers. + * allow changing the underlying type. * * Please note IT IS NOT SAFE to cast constness away if the result will ever * be modified (it would be undefined behaviour). Doing so anyway can cause @@ -1238,20 +1224,13 @@ typedef union PGAlignedXLogBlock #if defined(__cplusplus) #define unconstify(underlying_type, expr) const_cast<underlying_type>(expr) #define unvolatize(underlying_type, expr) const_cast<underlying_type>(expr) -#elif defined(HAVE__BUILTIN_TYPES_COMPATIBLE_P) +#else #define unconstify(underlying_type, expr) \ - (StaticAssertExpr(__builtin_types_compatible_p(__typeof(expr), const underlying_type), \ - "wrong cast"), \ + (StaticAssertExpr(_Generic((expr), const underlying_type: 1, default: 0), "wrong cast"), \ (underlying_type) (expr)) #define unvolatize(underlying_type, expr) \ - (StaticAssertExpr(__builtin_types_compatible_p(__typeof(expr), volatile underlying_type), \ - "wrong cast"), \ + (StaticAssertExpr(_Generic((expr), volatile underlying_type: 1, default: 0), "wrong cast"), \ (underlying_type) (expr)) -#else -#define unconstify(underlying_type, expr) \ - ((underlying_type) (expr)) -#define unvolatize(underlying_type, expr) \ - ((underlying_type) (expr)) #endif /* ---------------------------------------------------------------- diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index b0b0cfdaf79..4a25155239e 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -547,9 +547,6 @@ /* Define to 1 if your compiler understands __builtin_popcount. */ #undef HAVE__BUILTIN_POPCOUNT -/* Define to 1 if your compiler understands __builtin_types_compatible_p. */ -#undef HAVE__BUILTIN_TYPES_COMPATIBLE_P - /* Define to 1 if your compiler understands __builtin_unreachable. */ #undef HAVE__BUILTIN_UNREACHABLE -- 2.51.0
