Re: pgindent timing (was Re: [COMMITTERS] pgsql: Refactor NUM_cache_remove calls in error report path to a PG_TRY)

On Wed, Aug 12, 2009 at 12:27 AM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Aug 11, 2009 at 10:08 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> If that's not it, you'd need to mention details.
>
>> Well, one thing I've noticed is that when a function prototype wraps
>> around to the next line, you often change the number of spaces in the
>> hanging indent.
>
> Ah.  That's a bit idiosyncratic to pgindent.  What it does for a
> function definition makes sense, I think: it lines up all the
> parameters to start in the same column:
>
> static int
> myfunction(int foo,
>           int bar)
>
> What is not obvious is that the same amount of hanging indent is
> used for the function's *prototype*, even though the first line
> is not the same:
>
> static int myfunction(int foo,
>           int bar);
>
> ie, the indent is length of function name plus 1.  I find this a bit
> stupid myself and would prefer that the prototypes looked like this:
>
> static int myfunction(int foo,
>                      int bar);

That is truly bizarre. +1 from me for doing something that a
competent C programmer can figure out without a calculator. I don't
care what the rule is particularly, as long as it's obvious how to
follow it. (In my own code I indent all of my continuation lines by
one additional 4-space tab-stop. I realize this would be a horrible
idea for PG since we don't want to change anything that's going to
reindent the entire code base, and you might all hate it for other
reasons anyway, but the point is that any idiot can look at it and
figure out how it's supposed to be indented, because the rule is
simple.)

> which is what my editor will do with it if left to its own devices.
> So depending on whether I had occasion to touch the prototype, you might
> see it get reindented to the second style.  (I know pgindent will change
> it again, but I'm not sufficiently anal to override Emacs here.)  Also,
> I tend to make sure that there are not so many parameters declared on
> the same line that it would look bad if pgindent ever starts doing
> things my way, ie, if I see
>
> static someveryverylongtypename *somefunctionname(int foo,
>                 int bar, int baz, int quux, int somethingelse, int more);
>
> I'll probably split the second line, even though there's room for it
> according to pgindent's current rule.
>
> So, yeah, I'm being a bit anal about this and yet I'm not always
> consistent.  But that's what pgindent is good at fixing ...

I guess all I can say on this point is it might be helpful to people
if you avoid changing minor deviations from the pgindent standard
unless you're pretty sure that you're matching the standard exactly.

>> I am also a bit confused about the rule for indenting the variables in
>> variable declarations.  I get that the *, if any, is suppose to hang
>> to the left of the name, so that the first character of each variable
>> name lines up - and in particular, line up on a tab stop.  But it's
>> not obvious to me how to choose which tab stop to line everything up
>> on.
>
> I'm not too sure either.  It's the fourth tab stop for declarations at
> the outer level of a function, but I've never entirely figured out what
> pgindent's rule is for nested declarations.  It seems to be willing to
> move the target column over to some extent, but eventually it gives up
> and doesn't add any more whitespace.  I don't usually try very hard to
> align declarations exactly where pgindent will put them, although if
> it's visually ragged as-submitted sometimes I'll fix that.
>
> One thing I frequently *do* do is remove submitted whitespace changes
> if I know that pgindent would undo them anyway, just to reduce the size
> of the final diff.  A case that happened at least once in this
> commitfest is that somebody had taken a block of variables aligned to
> the fourth tab stop:
>
>    int         foo;
>    int         bar;
>    int         baz;
>
> and realigned them to the fifth stop, because he was adding another
> variable with a longer type name:
>
>    int             foo;
>    int             bar;
>    int             baz;
>    longtypename   *ptr;
>
> Well, that looks nice, but pgindent is going to do this anyway:
>
>    int         foo;
>    int         bar;
>    int         baz;
>    longtypename *ptr;
>
> so I tend to revert such dead-end whitespace changes just as part of
> making sure there wasn't an unintentional change of the existing lines.
> (This is the sort of thing that having submitters pgindent beforehand
> might avoid ...)

Yeah, I probably did that, because I didn't understand the rules.

> Bored yet?

No. This stuff really sucks. To give you another example of why it
sucks, consider that I often rebase my patch on top of your commit to
see what you changed, in an attempt to learn what I should do
differently next time. Of course I get a conflict every place where
you've made a change, which is the point, but the ones that are
whitespace-only make a big mess that makes it a lot harder to see what
has really changed. The possibility of understanding the indentation
will enough to minimize this sort of thing has my *undivided*
*attention*.

...Robert