Обсуждение: Spacing of options in getopt_long processing
Over at [1] Vaibhav complained that the patch was deleting a line following one of the case branches for handling command line options in pg_restore.c, and said this was not pertinent to the patch. That's reasonable, but it made me look into $subject a bit. pg_restore.c has a mixture, with some options being followed by blank lines and some not. pg_dumpall.c and pg_dump.c have a blank line after each option, while psql's startup.c has none. It would be nice to clean this up and have a consistent style. But what style? Personally I think having a blank line after each option looks cleaner, and we're not nearly so concerned with preserving vertical space as we might once have been. I haven't surveyed other utilities in our suite. Is this worth even pursuing? Do we care about making each file consistent, or making all the code consistent? cheers andrew [1] https://postgr.es/m/CA+vB=AE4SSSqRdPFWxe0zbq1n5ePo8iVHoXQGsu0Xr2srh_yDA@mail.gmail.com -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 04.11.25 20:32, Andrew Dunstan wrote: > Over at [1] Vaibhav complained that the patch was deleting a line > following one of the case branches for handling command line options in > pg_restore.c, and said this was not pertinent to the patch. That's > reasonable, but it made me look into $subject a bit. pg_restore.c has a > mixture, with some options being followed by blank lines and some not. > pg_dumpall.c and pg_dump.c have a blank line after each option, while > psql's startup.c has none. It would be nice to clean this up and have a > consistent style. But what style? Personally I think having a blank line > after each option looks cleaner, and we're not nearly so concerned with > preserving vertical space as we might once have been. I haven't surveyed > other utilities in our suite. Is this worth even pursuing? Do we care > about making each file consistent, or making all the code consistent? I think it depends. For example, looking through getopt_long() in initdb.c or pg_receivewal.c, each option processing is very simple. Would adding blank lines there add anything in terms of clarity? I doubt it. But then there is pg_resetwal.c, where each option processing is rather complex, and so the extra blank lines seem almost necessary. Along those lines, I would suggest that pg_waldump.c adds some blank lines, but perhaps pg_rewind.c could remove them. Only what pg_restore.c is doing is clearly wrong. ;-) I am mindful of the vertical space. Horizontal space is rather cheaper and the stuff toward the right is usually less important, but that doesn't apply vertically.
On 2025-11-05 We 11:41 AM, Peter Eisentraut wrote: > On 04.11.25 20:32, Andrew Dunstan wrote: >> Over at [1] Vaibhav complained that the patch was deleting a line >> following one of the case branches for handling command line options >> in pg_restore.c, and said this was not pertinent to the patch. That's >> reasonable, but it made me look into $subject a bit. pg_restore.c has >> a mixture, with some options being followed by blank lines and some >> not. pg_dumpall.c and pg_dump.c have a blank line after each option, >> while psql's startup.c has none. It would be nice to clean this up >> and have a consistent style. But what style? Personally I think >> having a blank line after each option looks cleaner, and we're not >> nearly so concerned with preserving vertical space as we might once >> have been. I haven't surveyed other utilities in our suite. Is this >> worth even pursuing? Do we care about making each file consistent, or >> making all the code consistent? > > I think it depends. For example, looking through getopt_long() in > initdb.c or pg_receivewal.c, each option processing is very simple. > Would adding blank lines there add anything in terms of clarity? I > doubt it. But then there is pg_resetwal.c, where each option > processing is rather complex, and so the extra blank lines seem almost > necessary. > > Along those lines, I would suggest that pg_waldump.c adds some blank > lines, but perhaps pg_rewind.c could remove them. > > Only what pg_restore.c is doing is clearly wrong. ;-) > > I am mindful of the vertical space. Horizontal space is rather > cheaper and the stuff toward the right is usually less important, but > that doesn't apply vertically. OK, I will clean up pg_restore.c and leave it at that. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2025-Nov-04, Andrew Dunstan wrote: > Over at [1] Vaibhav complained that the patch was deleting a line following > one of the case branches for handling command line options in pg_restore.c, > and said this was not pertinent to the patch. That's reasonable, but it made > me look into $subject a bit. pg_restore.c has a mixture, with some options > being followed by blank lines and some not. pg_dumpall.c and pg_dump.c have > a blank line after each option, while psql's startup.c has none. It would be > nice to clean this up and have a consistent style. But what style? > Personally I think having a blank line after each option looks cleaner, and > we're not nearly so concerned with preserving vertical space as we might > once have been. I haven't surveyed other utilities in our suite. Is this > worth even pursuing? Do we care about making each file consistent, or making > all the code consistent? I think not all switch blocks are the same. In getopt_long() switch blocks, I think the empty lines are mostly useless, because the 'break' plus the changing indentation for the next line is sufficient visual cue, and the vertical space _is_ worth more than zero. But if you go to more complex switches (say xact.c ones) then the empty lines are quite necessary, especially given pgindent's tendency to add indentation to comments immediately preceding a case line (but even without that). -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ […] indem ich in meinem Leben oft an euch gedacht, euch glücklich zu machen. Seyd es! A menudo he pensado en vosotros, en haceros felices. ¡Sedlo, pues! Heiligenstädter Testament, L. v. Beethoven, 1802 https://de.wikisource.org/wiki/Heiligenstädter_Testament