From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | pgsql-hackers(at)postgresql(dot)org |
Cc: | Thomas Munro <thomas(dot)munro(at)gmail(dot)com> |
Subject: | pg_attribute_noreturn(), MSVC, C11 |
Date: | 2024-12-13 19:10:13 |
Message-ID: | pxr5b3z7jmkpenssra5zroxi7qzzp6eswuggokw64axmdixpnk@zbwxuq7gbbcw |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
I just encountered another
warning C4715: 'XYZ: not all control paths return a value
with msvc in CI in a case where it should be trivial for the compiler to
recognize that the function return isn't reachable.
Which made me check if these days msvc has something like gcc's
__attribute__((noreturn)).
And it turns out that yes! The _Noreturn attribute has been added to C11 and
msvc supports C11:
https://learn.microsoft.com/en-us/cpp/c-language/noreturn?view=msvc-170
Besides the _Noreturn keyword the standard also added a stdnoreturn.h which
provides the 'noreturn' macro.
I first thought we could just implement pg_attribute_noreturn() using
_Noreturn if available. However, our current pg_attribute_noreturn() is in the
wrong place for that to work :(. _Noreturn is to be placed with the return
type, whereas function attributes with the __attribute__(()) syntax are after
the parameter list.
So we probably can't transparently switch between these.
C11 has been out a while, so I'm somewhat inclined to adopt _Noreturn/noreturn
in a conditional way. Older compilers would still work, just not understand
noreturn.
One wrinkle: _Noreturn/noreturn have been deprecated in C23, because that
adopted C++11's attribute syntax (i.e. [[noreturn]]). But that's at least in
the same place as _Noreturn/return.
We can't remove [[noreturn]] with preprocessor magic, so it's not really
viable to use that for, uhm, quite a while.
If we were to use _Noreturn, I think it could just be something like:
I think it should suffice to do something like
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
#define pg_noreturn _Noreturn
#else
#define pg_noreturn
#endif
(or alternatively include stdreturn if __STDC_VERSION__ indicates support and
define a bare 'noreturn' if not)
For msvc that mean we'd need to add /std:c11 (or /std:c17) to the compiler
flags, as it otherwise it results in a weird mix of c89 an c99). But that
might be a good idea anyway. With one minor change [1] the tests pass with
msvc when using /std:c17.
Before realizing that we'd have to change our existing annotations and that
there's no way to use both old and new syntax, depending on compiler support,
I was thinking this would be an obvious thing to do. I'm still leaning on it
being worth it, but not as clearly as before.
For an example of _Noreturn being used: https://godbolt.org/z/j15d35dao
Greetings,
Andres Freund
[1] The msvc implementation of VA_ARGS_NARGS only works with the traditional
preprocessor, but C11 support enables the new one. But we can detect that
with something like
(!defined(_MSVC_TRADITIONAL) || _MSVC_TRADITIONAL)
See https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170
and https://learn.microsoft.com/en-us/cpp/build/reference/std-specify-language-standard-version?view=msvc-170
Without adapting the definition of VA_ARGS_NARGS compilation fails as it
results in the macro resulting in VA_ARGS_NARGS_(prefix, 63, 62, ...) in
the using code...
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2024-12-13 19:54:11 | Re: pg_attribute_noreturn(), MSVC, C11 |
Previous Message | Jeff Davis | 2024-12-13 17:07:54 | Re: fixing tsearch locale support |