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.)