Thread: Re: [COMMITTERS] pgsql: Add macros wrapping all usage of gcc's __attribute__.

Andres Freund <andres@anarazel.de> writes:
> Add macros wrapping all usage of gcc's __attribute__.

I noticed that this commit attached pg_attribute_noreturn not only
to the extern declarations, but to some actual function definitions.
I think this is a bad idea, because it's going to look like heck after
pgindent gets through with it.  Do we actually need decoration on the
function definitions?
        regards, tom lane

On 2015-03-25 19:11:06 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Add macros wrapping all usage of gcc's __attribute__.
> 
> I noticed that this commit attached pg_attribute_noreturn not only
> to the extern declarations, but to some actual function definitions.

Unless either Oskari or I screwed up, it should just have been a 1:1
translation from previous __attribute__((noreturn)) to
pg_attribute_noreturn. I looked through the commit just now and didn't
see any new locations.

> I think this is a bad idea, because it's going to look like heck after
> pgindent gets through with it.  Do we actually need decoration on the
> function definitions?

Hm, I guess it should not look any worse than before? None of the
locations look like they've been introduced after the last pgindent
run. I only see plpgsql_yyerror, yyerror. That said, I see little reason
to add the noreturn thingy to the definition and not the declaration for
those.  It actually looks to me like there's a declaration for
replication_yyerror, but a plain yyerror is used instead in repl_scanner.l?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2015-03-25 19:11:06 -0400, Tom Lane wrote:
>> I think this is a bad idea, because it's going to look like heck after
>> pgindent gets through with it.  Do we actually need decoration on the
>> function definitions?

> Hm, I guess it should not look any worse than before?

It does, because pgindent treats "pg_attribute_noreturn" differently
than it treated "__attribute__((noreturn))".  Before you'd end up
with something like

void
__attribute__((noreturn))
plpgsql_yyerror(const char *message)
{

pgindent forced the __attribute__(()) bit onto its own line, whether you
wrote it that way or not, but it doesn't look *too* awful.  But now that
becomes:

void        pg_attribute_noreturn
plpgsql_yyerror(const char *message)
{

The best you can get is to manually put the noreturn back onto the
"void" line, but you still end up with:

void        pg_attribute_noreturn
plpgsql_yyerror(const char *message)
{

So this is just ugly.  Maybe we could teach pgindent not to do that,
but I'm doubtful.

> ... That said, I see little reason
> to add the noreturn thingy to the definition and not the declaration for
> those.  It actually looks to me like there's a declaration for
> replication_yyerror, but a plain yyerror is used instead in repl_scanner.l?

Right.

Also, even in the context of extern declarations, it seems to be a lot
easier to get pgindent not to mess with your layout if
"pg_attribute_noreturn" is replaced with "pg_attribute_noreturn()".
I see no particular reason not to add parens to the macro, do you?

Being the one complaining, I'll go do the legwork to clean this up.
        regards, tom lane

On 2015-03-26 11:27:32 -0400, Tom Lane wrote:
> Being the one complaining, I'll go do the legwork to clean this up.

Looks good, Thanks!

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services