pg_attribute_noreturn(), MSVC, C11 - Mailing list 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...