Re: [HACKERS] pgindent behavior we could do without
On Thu, Jan 30, 2014 at 11:44:31PM -0500, Tom Lane wrote: Bruce Momjian br...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent behavior we could do without
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent behavior we could do without
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 WIN32 static intcopy_file(const char *fromfile, const char *tofile, bool force); -- #else static intwin32_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 intcopy_file(const char *fromfile, const char *tofile, bool force); ... ... ... #else static intwin32_pghardlink(const char *src, const char *dst); ... ... ... #endif Ideas? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent behavior we could do without
Bruce Momjian br...@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 intcopy_file(const char *fromfile, const char *tofile, bool force); -- #else static intwin32_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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent behavior we could do without
On Thu, Jan 30, 2014 at 01:46:34PM -0500, Tom Lane wrote: Bruce Momjian br...@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 intcopy_file(const char *fromfile, const char *tofile, bool force); -- #else static intwin32_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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent behavior we could do without
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 br...@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 intcopy_file(const char *fromfile, const char *tofile, bool force); -- #else static intwin32_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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent behavior we could do without
Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent behavior we could do without
On 07/18/2013 11:30 PM, Tom Lane wrote: Noah Misch n...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent behavior we could do without
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent behavior we could do without
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgindent behavior we could do without
Noah Misch n...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
