Re: pgindent && weirdness

On 16/01/2020 03.59, Thomas Munro wrote:
> On Wed, Jan 15, 2020 at 11:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>>> I just ran pgindent over some patch, and noticed that this hunk ended up
>>> in my working tree:
>>
>>> -     if (IsA(leftop, Var) && IsA(rightop, Const))
>>> +     if (IsA(leftop, Var) &&IsA(rightop, Const))
>>
>> Yeah, it's been doing that for decades.  I think the triggering
>> factor is the typedef name (Var, here) preceding the &&.
>>
>> It'd be nice to fix properly, but I've tended to take the path
>> of least resistance by breaking such lines to avoid the ugliness:
>>
>>          if (IsA(leftop, Var) &&
>>              IsA(rightop, Const))
> 
> I am on vacation away from the Internet this week but somehow saw this
> on my phone and couldn't stop myself from peeking at pg_bsd_ident
> again. Yeah, "(Var)" (where Var is a known typename) causes it to
> think that any following operator must be unary.
> 
> One way to fix that in the cases Alvaro is referring to is to tell
> override the setting so that && (and likewise ||) are never considered
> to be unary, though I haven't tested this much and there are surely
> other ways to achieve this:
> 
> diff --git a/lexi.c b/lexi.c
> index d43723c..6de3227 100644
> --- a/lexi.c
> +++ b/lexi.c
> @@ -655,6 +655,12 @@ stop_lit:
>              unary_delim = state->last_u_d;
>              break;
>          }
> +
> +       /* && and || are never unary */
> +       if ((token[0] == '&' && *buf_ptr == '&') ||
> +               (token[0] == '|' && *buf_ptr == '|'))
> +               state->last_u_d = false;
> +
>          while (*(e_token - 1) == *buf_ptr || *buf_ptr == '=') {
>              /*
>               * handle ||, &&, etc, and also things as in int *****i
> 
> The problem with that is that && sometimes *should* be formatted like
> a unary operator: when it's part of the nonstandard GCC computed goto
> syntax.

These comments are made in the context of pushing this change or 
equivalent to FreeBSD repository.

I think this is a better approach then the one committed to 
pg_bsd_indent. It's ubiquitous that the operators are binary, except - 
as you mentioned - in a nonstandard GCC syntax. The alternative given 
has more disadvantages, with potential impact on FreeBSD code 
formatting, which it should support as well as everything else -- to a 
reasonable extent. sys/kern/ is usually a decent litmus test, but I 
don't claim it should show anything interesting in this particular case.

This change may seem hacky, but it would be far from the worst hack in 
this program's history or even in its present form. It's actually very 
much in indent's spirit, which is an attribute I neither support nor 
condemn.

In any case, this change, or equivalent, should be committed to FreeBSD 
repository together with a test case or two.