Re: Running pgindent

* 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:

*** 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?

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

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.

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.

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

Thanks,

Stephen