Hi,
At the bottom of
https://www.postgresql.org/message-id/20191112192716.emrqs2afuefunw6v%40alap3.anarazel.de
I mused about the somewhat odd coding pattern at the end of WalSndShutdown():
/*
* Handle a client's connection abort in an orderly manner.
*/
static void
WalSndShutdown(void)
{
/*
* Reset whereToSendOutput to prevent ereport from attempting to send any
* more messages to the standby.
*/
if (whereToSendOutput == DestRemote)
whereToSendOutput = DestNone;
proc_exit(0);
abort(); /* keep the compiler quiet */
}
namely that we prox_exit() and then abort(). This case seems to be
purely historical baggage (from when this was inside other functiosn,
before being centralized), so we can likely just remove the abort().
But even back then, one would have hoped that proc_exit() being
annotated with pg_attribute_noreturn() should have told the compiler
enough.
But it turns out, we don't actually implement that for MSVC. Which does
explain at least some cases where we had to add "keep compiler quiet"
type code specifically for MSVC.
As it turns out msvc has it's own annotation for functions that don't
return, __declspec(noreturn). But it unfortunately needs to be placed
before where we, so far, placed pg_attribute_noreturn(), namely after
the function name / parameters. Instead it needs to be before the
function name.
But as it turns out GCC et al's __attribute__((noreturn)) actually can
also be placed there, and seemingly for a long time:
https://godbolt.org/z/8v5aFS
So perhaps we ought to rename pg_attribute_noreturn() to pg_noreturn(),
add a __declspec(noreturn) version, and move the existing uses to it.
I'm inclined to also drop the parentheses at the same time (i.e
pg_noreturn rather than pg_noreturn()) - it seems easier to mentally
parse the code that way.
I actually find the placement closer to the return type easier to
understand, so I'd find this mildly beneficial even without the msvc
angle.
Greetings,
Andres Freund
