Обсуждение: pgindent behavior we could do without

It's always annoyed me that pgindent insists on adjusting vertical
whitespace around #else and related commands.  This has, for example,
rendered src/include/storage/barrier.h nigh illegible: you get things
like

/** lwsync orders loads with respect to each other, and similarly with stores.* But a load can be performed before a
subsequentstore, so sync must be used* for a full memory barrier.*/
 
#define pg_memory_barrier()     __asm__ __volatile__ ("sync" : : : "memory")
#define pg_read_barrier()       __asm__ __volatile__ ("lwsync" : : : "memory")
#define pg_write_barrier()      __asm__ __volatile__ ("lwsync" : : : "memory")
#elif defined(__alpha) || defined(__alpha__)    /* Alpha */

which makes it look like this block of code has something to do with
Alpha.

By chance, I noticed today that this misbehavior comes from a discretely
identifiable spot, to wit lines 289-290 in src/tools/pgindent/pgindent:
# Remove blank line(s) before #else, #elif, and #endif$source =~ s!\n\n+(\#else|\#elif|\#endif)!\n$1!g;

This seems pretty broken to me: why exactly is whitespace there such a
bad idea?  Not only that, but the next action is concerned with undoing
some of the damage this rule causes:
# Add blank line before #endif if it is the last line in the file$source =~ s!\n(#endif.*)\n\z!\n\n$1\n!;

I assert that we should simply remove both of these bits of code, as
just about every committer on the project is smarter about when to use
vertical whitespace than this program is.

Thoughts?
        regards, tom lane

On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
> It's always annoyed me that pgindent insists on adjusting vertical
> whitespace around #else and related commands.  This has, for example,
> rendered src/include/storage/barrier.h nigh illegible: you get things
> like
> 
> /*
>  * lwsync orders loads with respect to each other, and similarly with stores.
>  * But a load can be performed before a subsequent store, so sync must be used
>  * for a full memory barrier.
>  */
> #define pg_memory_barrier()     __asm__ __volatile__ ("sync" : : : "memory")
> #define pg_read_barrier()       __asm__ __volatile__ ("lwsync" : : : "memory")
> #define pg_write_barrier()      __asm__ __volatile__ ("lwsync" : : : "memory")
> #elif defined(__alpha) || defined(__alpha__)    /* Alpha */
> 
> which makes it look like this block of code has something to do with
> Alpha.
> 
> By chance, I noticed today that this misbehavior comes from a discretely
> identifiable spot, to wit lines 289-290 in src/tools/pgindent/pgindent:
> 
>     # Remove blank line(s) before #else, #elif, and #endif
>     $source =~ s!\n\n+(\#else|\#elif|\#endif)!\n$1!g;
> 
> This seems pretty broken to me: why exactly is whitespace there such a
> bad idea?  Not only that, but the next action is concerned with undoing
> some of the damage this rule causes:
> 
>     # Add blank line before #endif if it is the last line in the file
>     $source =~ s!\n(#endif.*)\n\z!\n\n$1\n!;
> 
> I assert that we should simply remove both of these bits of code, as
> just about every committer on the project is smarter about when to use
> vertical whitespace than this program is.
> 
> Thoughts?

Interesting.  I can't remember fully but the problem might be that BSD
indent was adding extra spacing around those, and I needed to remove it.
Would you like me to test a run and show you the output?  I can do that
next week when I return from vacation, or you can give it a try.  I am
fine with removing that code.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +

On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
> It's always annoyed me that pgindent insists on adjusting vertical
> whitespace around #else and related commands.  This has, for example,
> rendered src/include/storage/barrier.h nigh illegible: you get things
> like
> 
> /*
>  * lwsync orders loads with respect to each other, and similarly with stores.
>  * But a load can be performed before a subsequent store, so sync must be used
>  * for a full memory barrier.
>  */
> #define pg_memory_barrier()     __asm__ __volatile__ ("sync" : : : "memory")
> #define pg_read_barrier()       __asm__ __volatile__ ("lwsync" : : : "memory")
> #define pg_write_barrier()      __asm__ __volatile__ ("lwsync" : : : "memory")
> #elif defined(__alpha) || defined(__alpha__)    /* Alpha */
> 
> which makes it look like this block of code has something to do with
> Alpha.

Agreed.  I've similarly disliked how pgindent adds a blank line between an #if
and a multi-line comment, like at the top of get_restricted_token().

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com

Noah Misch <noah@leadboat.com> writes:
> On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
>> It's always annoyed me that pgindent insists on adjusting vertical
>> whitespace around #else and related commands.  This has, for example,
>> rendered src/include/storage/barrier.h nigh illegible: you get things
>> like

> Agreed.  I've similarly disliked how pgindent adds a blank line between an #if
> and a multi-line comment, like at the top of get_restricted_token().

AFAICT it forces a blank line before a multiline comment in most
contexts; #if isn't special (though it does seem to avoid doing that
just after a left brace).  I too don't much care for that behavior,
although it's not as detrimental to readability as removing blank lines
can be.

In general, pgindent isn't nearly as good about managing vertical
whitespace as it is about horizontal spacing ...
        regards, tom lane

On 07/18/2013 11:30 PM, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
>> On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
>>> It's always annoyed me that pgindent insists on adjusting vertical
>>> whitespace around #else and related commands.  This has, for example,
>>> rendered src/include/storage/barrier.h nigh illegible: you get things
>>> like
>> Agreed.  I've similarly disliked how pgindent adds a blank line between an #if
>> and a multi-line comment, like at the top of get_restricted_token().
> AFAICT it forces a blank line before a multiline comment in most
> contexts; #if isn't special (though it does seem to avoid doing that
> just after a left brace).  I too don't much care for that behavior,
> although it's not as detrimental to readability as removing blank lines
> can be.
>
> In general, pgindent isn't nearly as good about managing vertical
> whitespace as it is about horizontal spacing ...
>

I agree.

I should perhaps note that when I rewrote pgindent, I deliberately 
didn't editorialize about its behaviour - but I did intend to make it 
more maintainable and simpler to change stuff like this, and so it is :-)

cheers

andrew

On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
> It's always annoyed me that pgindent insists on adjusting vertical
> whitespace around #else and related commands.  This has, for example,
> rendered src/include/storage/barrier.h nigh illegible: you get things
> like
> 
> /*
>  * lwsync orders loads with respect to each other, and similarly with stores.
>  * But a load can be performed before a subsequent store, so sync must be used
>  * for a full memory barrier.
>  */
> #define pg_memory_barrier()     __asm__ __volatile__ ("sync" : : : "memory")
> #define pg_read_barrier()       __asm__ __volatile__ ("lwsync" : : : "memory")
> #define pg_write_barrier()      __asm__ __volatile__ ("lwsync" : : : "memory")
> #elif defined(__alpha) || defined(__alpha__)    /* Alpha */
> 
> which makes it look like this block of code has something to do with
> Alpha.
> 
> By chance, I noticed today that this misbehavior comes from a discretely
> identifiable spot, to wit lines 289-290 in src/tools/pgindent/pgindent:
> 
>     # Remove blank line(s) before #else, #elif, and #endif
>     $source =~ s!\n\n+(\#else|\#elif|\#endif)!\n$1!g;
> 
> This seems pretty broken to me: why exactly is whitespace there such a
> bad idea?  Not only that, but the next action is concerned with undoing
> some of the damage this rule causes:
> 
>     # Add blank line before #endif if it is the last line in the file
>     $source =~ s!\n(#endif.*)\n\z!\n\n$1\n!;
> 
> I assert that we should simply remove both of these bits of code, as
> just about every committer on the project is smarter about when to use
> vertical whitespace than this program is.

OK, I have developed the attached patch that shows the code change of
removing the test for #else/#elif/#endif.  You will see that the new
output has odd blank lines for cases like:
#ifndef WIN32static int    copy_file(const char *fromfile, const char *tofile, bool force);
-->#elsestatic int    win32_pghardlink(const char *src, const char *dst);
-->#endif

I can't think of a way to detect tight blocks like the above from cases
where there are sizable blocks, like:
#ifndef WIN32
static int    copy_file(const char *fromfile, const char *tofile, bool force);.........#else
static int    win32_pghardlink(const char *src, const char *dst);.........
#endif

Ideas?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

Bruce Momjian <bruce@momjian.us> writes:
> On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
>> I assert that we should simply remove both of these bits of code, as
>> just about every committer on the project is smarter about when to use
>> vertical whitespace than this program is.

> OK, I have developed the attached patch that shows the code change of
> removing the test for #else/#elif/#endif.  You will see that the new
> output has odd blank lines for cases like:

>     #ifndef WIN32
>     static int    copy_file(const char *fromfile, const char *tofile, bool force);
> -->
>     #else
>     static int    win32_pghardlink(const char *src, const char *dst);
> -->
>     #endif

Hm.  So actually, that code is trying to undo excess vertical whitespace
that something earlier in pgindent added?  Where is that happening?
        regards, tom lane

On Thu, Jan 30, 2014 at 01:46:34PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
> >> I assert that we should simply remove both of these bits of code, as
> >> just about every committer on the project is smarter about when to use
> >> vertical whitespace than this program is.
> 
> > OK, I have developed the attached patch that shows the code change of
> > removing the test for #else/#elif/#endif.  You will see that the new
> > output has odd blank lines for cases like:
> 
> >     #ifndef WIN32
> >     static int    copy_file(const char *fromfile, const char *tofile, bool force);
> > -->
> >     #else
> >     static int    win32_pghardlink(const char *src, const char *dst);
> > -->
> >     #endif
> 
> Hm.  So actually, that code is trying to undo excess vertical whitespace
> that something earlier in pgindent added?  Where is that happening?

I am afraid it is _inside_ BSD indent, and if ever looked at that code,
you would not want to go in there.  :-O

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Thu, Jan 30, 2014 at 01:52:55PM -0500, Bruce Momjian wrote:
> On Thu, Jan 30, 2014 at 01:46:34PM -0500, Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > > On Thu, Jul 18, 2013 at 12:27:21AM -0400, Tom Lane wrote:
> > >> I assert that we should simply remove both of these bits of code, as
> > >> just about every committer on the project is smarter about when to use
> > >> vertical whitespace than this program is.
> > 
> > > OK, I have developed the attached patch that shows the code change of
> > > removing the test for #else/#elif/#endif.  You will see that the new
> > > output has odd blank lines for cases like:
> > 
> > >     #ifndef WIN32
> > >     static int    copy_file(const char *fromfile, const char *tofile, bool force);
> > > -->
> > >     #else
> > >     static int    win32_pghardlink(const char *src, const char *dst);
> > > -->
> > >     #endif
> > 
> > Hm.  So actually, that code is trying to undo excess vertical whitespace
> > that something earlier in pgindent added?  Where is that happening?
> 
> I am afraid it is _inside_ BSD indent, and if ever looked at that code,
> you would not want to go in there.  :-O

OK, seven hours later, I have fixed pg_bsd_indent to no longer insert
blank lines above #elif/#else/#endif, and therefore removed the special
case code from pgindent.

You will need to download version 1.3 of pg_bsd_indent for this to work,
and pgindent will complain if it doesn't find the right pg_bsd_indent
version.

Do we need to go an clean up any places where pgindent has suppressed
blank lines above #elif/#else/#endif in the past?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

Bruce Momjian <bruce@momjian.us> writes:
> OK, seven hours later, I have fixed pg_bsd_indent to no longer insert
> blank lines above #elif/#else/#endif, and therefore removed the special
> case code from pgindent.

> You will need to download version 1.3 of pg_bsd_indent for this to work,
> and pgindent will complain if it doesn't find the right pg_bsd_indent
> version.

Cool.

> Do we need to go an clean up any places where pgindent has suppressed
> blank lines above #elif/#else/#endif in the past?

Not sure it's worth making a massive change for this, but I appreciate the
fact that pgindent won't mess up such code in the future.
        regards, tom lane

On Thu, Jan 30, 2014 at 11:44:31PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > OK, seven hours later, I have fixed pg_bsd_indent to no longer insert
> > blank lines above #elif/#else/#endif, and therefore removed the special
> > case code from pgindent.
> 
> > You will need to download version 1.3 of pg_bsd_indent for this to work,
> > and pgindent will complain if it doesn't find the right pg_bsd_indent
> > version.
> 
> Cool.
> 
> > Do we need to go an clean up any places where pgindent has suppressed
> > blank lines above #elif/#else/#endif in the past?
> 
> Not sure it's worth making a massive change for this, but I appreciate the
> fact that pgindent won't mess up such code in the future.

Yes, it is a shame pgindent has removed many proper empty lines in the
past and there is no way to re-add them without causing backpatching
problems.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Fri, Jan 31, 2014 at 11:18:17AM -0500, Bruce Momjian wrote:
> Yes, it is a shame pgindent has removed many proper empty lines in the
> past and there is no way to re-add them without causing backpatching
> problems.

FYI, the original BSD indent code that added the blank lines kind of
made sense.  If you defined a block of variables or a function, BSD
indent wanted a blank line after that.  When it saw a CPP directive, it
knew that wasn't a blank line, so it forced one.

In our coding, we often want CPP directives with no blank space, hence
the problem.  pg_bsd_indent 1.3 will not longer force a blank line
when it sees a CPP directive, and pgindent will no longer remove those
blank lines.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +