Re: Running pgindent

On Wed, May 29, 2013 at 10:08:10PM -0400, Stephen Frost wrote:
> * Bruce Momjian (bruce(at)momjian(dot)us) wrote:
> > Wow, uh, yeah, I guess we could do that. I will await more feedback.
>
> Please don't. I'm already rather concerned by this one. It looks like
> there's a rule to pull a line in to meet the max-column requirement even
> when that makes things line up 'funny', eg:

I did a comparison of the parameters passed to BSD indent, and Andrew
did accurately transfer all the flags, so I am a little confused why
there is such a difference, as BSD indent has not changed.

> *** a/contrib/hstore/hstore_io.c
> --- b/contrib/hstore/hstore_io.c
> *************** hstore_to_json_loose(PG_FUNCTION_ARGS)
> *** 1300,1306 ****
> * digit as numeric - could be a zip code or similar
> */
> if (src->len > 0 &&
> ! !(src->data[0] == '0' && isdigit((unsigned char) src->data[1])) &&
> strspn(src->data, "+-0123456789Ee.") == src->len)
> {
> /*
> --- 1300,1306 ----
> * digit as numeric - could be a zip code or similar
> */
> if (src->len > 0 &&
> ! !(src->data[0] == '0' && isdigit((unsigned char) src->data[1])) &&
> strspn(src->data, "+-0123456789Ee.") == src->len)
> {
> /*
>
> For this case, perhaps we should just split it on to multiple lines, but
> when there's not much on the line beyond a long string, I thought our
> policy was to allow the line to go beyond the column limit? That's
> certainly how section 49.1 reads to me; any chance we can get indent to
> understand that?

I thought we used to do that too. However, I don't see this line in the
9.2 hstore source, so I assume it is new and that pgindent is doing what
it used to do.

> We also claim that our indent runs won't screw around with comment
> blocks, but it looks to pretty clearly be doing exactly that..

We only promise that if you use /* ---- markers.

> Another interesting case is this:
>
> *************** start_postmaster(ClusterInfo *cluster, b
> *** 229,235 ****
> * it might supply a reason for the failure.
> */
> pg_ctl_return = exec_prog(SERVER_START_LOG_FILE,
> ! /* pass both file names if they differ */
> (strcmp(SERVER_LOG_FILE,
> SERVER_START_LOG_FILE) != 0) ?
> SERVER_LOG_FILE : NULL,
> --- 229,235 ----
> * it might supply a reason for the failure.
> */
> pg_ctl_return = exec_prog(SERVER_START_LOG_FILE,
> ! /* pass both file names if they differ */
> (strcmp(SERVER_LOG_FILE,
> SERVER_START_LOG_FILE) != 0) ?
> SERVER_LOG_FILE : NULL,
>
> It seems that a comment inside of parameters passed to a function isn't
> indented to the same depth of the other arguments by this run, but I'm
> guessing it was by prior versions.

Uh, this comment was definitely in 9.2, but was added as a backpatch by
Tom in September 2012, after pg_upgrade was run on the 9.2 code.

> Also doesn't seem to indent operations the same way that function
> parameters are indented, eg:
>
> *************** pgrowlocks(PG_FUNCTION_ARGS)
> *** 166,172 ****
> values[Atnum_ismulti] = pstrdup("true");
>
> allow_old = !(infomask & HEAP_LOCK_MASK) &&
> ! (infomask & HEAP_XMAX_LOCK_ONLY);
> nmembers = GetMultiXactIdMembers(xmax, &members, allow_old);
> if (nmembers == -1)
> {
> --- 166,172 ----
> values[Atnum_ismulti] = pstrdup("true");
>
> allow_old = !(infomask & HEAP_LOCK_MASK) &&
> ! (infomask & HEAP_XMAX_LOCK_ONLY);
> nmembers = GetMultiXactIdMembers(xmax, &members, allow_old);
> if (nmembers == -1)
> {
>
> Which seems to also be 'wrong' to my eyes.

Yes, it certainly does, but this line also does not exist in 9.2 source
tree.

> In general, there are a lot of improvements being made, but I don't like
> what appear to be regressions. :)

I agree. Can someone fine a case that was run through 9.2 pgindent that
is worse now? Every case I could find either was the same in 9.2 or was
changed after the 9.2 pgindent run.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +