Re: pg_attribute_noreturn(), MSVC, C11

On 14.12.24 18:18, Peter Eisentraut wrote:
> On 13.12.24 20:54, Andres Freund wrote:
>> Another wrinkle: While __attribute__((noreturn)) works for function
>> pointers
>> (or function pointer typedefs) _Noreturn doesn't. Gah.  We only use it
>> that
>> way in two places, but still :(
>
> Yeah, I wrote an experimental patch for noreturn support some years ago,
> and that was also my result back then.  (I assume you have a current
> patch, otherwise I can dig out that one.)  I had also written down that
> there were some problems with Perl and Tcl headers, FWIW.  Did you have
> any problems with those?
>
> I think we can take a small loss here and move with the standard. Unless
> you can think of a way to define pg_noreturn_but_for_function_pointers
> in a systematic way.

I have a patch proposal here. I discovered a few more complications
that need to be paid attention to.

First, we can't use bare "noreturn". There are third-party header files
(such as Tcl) that use __attribute__((noreturn)), and that will get
confused. That's the same reason we don't use bare restrict but
pg_restrict.

I suggest we define pg_noreturn as

1. If C11 is supported, then _Noreturn, else
2. If GCC-compatible, then __attribute__((noreturn)), else
3. If MSVC, then __declspec((noreturn))

When PostgreSQL starts requiring C11, then the latter two options can be
dropped.

Note that this also fixes a possible conflict where some third-party
code includes <stdnoreturn.h> and then includes our c.h. I don't think
this has been reported, but it's surely bound to happen.

For the function pointers, I don't think there is a good solution. The
current behavior is evidently inconsistent and incompatible and not well
documented, and I don't see how it's going to get better in these
aspects any time soon. I think the way forward in the mid-term is to
avoid designing more interfaces like that and provide wrapper functions
like json_manifest_parse_failure() where you can enforce the return
behavior in the normal way.

Finally, while I was looking at this, I figured we could also add
pg_nodiscard support to non-GCC compilers that support C23 (probably
none right now, but eventually), by defining pg_nodiscard as
[[nodiscard]]. But that revealed that clang requires these attributes
to appear before the extern/static keywords, which is not how it's
currently written. So I changed that, too, and also wrote the
pg_noreturn patch in the same style. And then I also added a definition
of pg_noreturn as [[noreturn]] (actually, [[__noreturn__]], for the
reasons given earlier), for consistency, and also as a hedge in case
some compiler drops C11 support in a few decades. (I tried to
language-lawyer my way through this, and I don't know that clang is
correct here, but this issue is widely reported out there and everyone
agrees that the fix is to just swap the things around. At least we can
enforce some stylistic consistency that way.)