pgindent && weirdness

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

pgindent && weirdness

Alvaro Herrera-9
I just ran pgindent over some patch, and noticed that this hunk ended up
in my working tree:

diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index 861a9148ed..fff54062b0 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -1405,13 +1405,13 @@ examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool *varonl
  if (IsA(rightop, RelabelType))
  rightop = (Node *) ((RelabelType *) rightop)->arg;
 
- if (IsA(leftop, Var) && IsA(rightop, Const))
+ if (IsA(leftop, Var) &&IsA(rightop, Const))
  {
  var = (Var *) leftop;
  cst = (Const *) rightop;
  varonleft = true;
  }
- else if (IsA(leftop, Const) && IsA(rightop, Var))
+ else if (IsA(leftop, Const) &&IsA(rightop, Var))
  {
  var = (Var *) rightop;
  cst = (Const *) leftop;

This seems a really strange change; this
git grep '&&[^([:space:]]' -- *.c
shows that we already have a dozen or so occurrences already.  (That's
ignoring execExprInterp.c's use of computed gotos.)

I don't care all that much, but wanted to throw it out in case somebody
is specifically interested in studying pgindent's logic, since the last
round of changes has yielded excellent results.

Thanks,

--
Álvaro Herrera                PostgreSQL Expert, https://www.2ndQuadrant.com/


Reply | Threaded
Open this post in threaded view
|

Re: pgindent && weirdness

Tom Lane-2
Alvaro Herrera <[hidden email]> 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))

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgindent && weirdness

Bruce Momjian
On Tue, Jan 14, 2020 at 05:30:21PM -0500, Tom Lane wrote:

> Alvaro Herrera <[hidden email]> 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))

In the past I would use a post-processing step after BSD indent to fix
up these problems.

--
  Bruce Momjian  <[hidden email]>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +


Reply | Threaded
Open this post in threaded view
|

Re: pgindent && weirdness

Thomas Munro-5
In reply to this post by Tom Lane-2
On Wed, Jan 15, 2020 at 11:30 AM Tom Lane <[hidden email]> wrote:

> Alvaro Herrera <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

Re: pgindent && weirdness

Thomas Munro-5
On Thu, Jan 16, 2020 at 3:59 PM Thomas Munro <[hidden email]> wrote:
> On Wed, Jan 15, 2020 at 11:30 AM Tom Lane <[hidden email]> wrote:
> > Yeah, it's been doing that for decades.  I think the triggering
> > factor is the typedef name (Var, here) preceding the &&.

Here's a better fix:

diff --git a/indent.c b/indent.c
index 9faf57a..51a60a6 100644
--- a/indent.c
+++ b/indent.c
@@ -570,8 +570,11 @@ check_type:
                ps.in_or_st = false;    /* turn off flag for structure decl or
                                         * initialization */
            }
-           /* parenthesized type following sizeof or offsetof is not a cast */
-           if (ps.keyword == 1 || ps.keyword == 2)
+           /*
+                * parenthesized type following sizeof or offsetof is
not a cast;
+                * likewise for function-like macros that take a type
+                */
+           if (ps.keyword == 1 || ps.keyword == 2 || ps.last_token == ident)
                ps.not_cast_mask |= 1 << ps.p_l_follow;
            break;


Reply | Threaded
Open this post in threaded view
|

Re: pgindent && weirdness

Alvaro Herrera-9
On 2020-Jan-17, Thomas Munro wrote:

> On Thu, Jan 16, 2020 at 3:59 PM Thomas Munro <[hidden email]> wrote:
> > On Wed, Jan 15, 2020 at 11:30 AM Tom Lane <[hidden email]> wrote:
> > > Yeah, it's been doing that for decades.  I think the triggering
> > > factor is the typedef name (Var, here) preceding the &&.
>
> Here's a better fix:

This is indeed a very good fix!  Several badly formatted sites in our
code are improved with this change.

Thanks,

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: pgindent && weirdness

Michael Paquier-2
On Thu, Jan 16, 2020 at 06:13:36PM -0300, Alvaro Herrera wrote:
> This is indeed a very good fix!  Several badly formatted sites in our
> code are improved with this change.

Nice find!  Could you commit that?  I can see many places improved as
well, among explain.c, tablecmds.c, typecmds.c, and much more.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgindent && weirdness

Alvaro Herrera-9
On 2020-Jan-17, Michael Paquier wrote:

> On Thu, Jan 16, 2020 at 06:13:36PM -0300, Alvaro Herrera wrote:
> > This is indeed a very good fix!  Several badly formatted sites in our
> > code are improved with this change.
>
> Nice find!  Could you commit that?  I can see many places improved as
> well, among explain.c, tablecmds.c, typecmds.c, and much more.

I think Tom is the only one who can commit that,
https://git.postgresql.org/gitweb/?p=pg_bsd_indent.git

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: pgindent && weirdness

Tom Lane-2
Alvaro Herrera <[hidden email]> writes:
> On 2020-Jan-17, Michael Paquier wrote:
>> Nice find!  Could you commit that?  I can see many places improved as
>> well, among explain.c, tablecmds.c, typecmds.c, and much more.

> I think Tom is the only one who can commit that,
> https://git.postgresql.org/gitweb/?p=pg_bsd_indent.git

I don't *think* that repo is locked down that hard --- IIRC,
PG committers should have access to it.  But I was hoping to
hear Piotr's opinion before moving forward.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pgindent && weirdness

Alvaro Herrera-9
On 2020-Jan-16, Tom Lane wrote:

> Alvaro Herrera <[hidden email]> writes:
> > On 2020-Jan-17, Michael Paquier wrote:
> >> Nice find!  Could you commit that?  I can see many places improved as
> >> well, among explain.c, tablecmds.c, typecmds.c, and much more.
>
> > I think Tom is the only one who can commit that,
> > https://git.postgresql.org/gitweb/?p=pg_bsd_indent.git
>
> I don't *think* that repo is locked down that hard --- IIRC,
> PG committers should have access to it.  But I was hoping to
> hear Piotr's opinion before moving forward.

FWIW I think this code predates Piotr's involvement, I think; at least,
it was already there in the FreeBSD code he imported:
https://github.com/pstef/freebsd_indent/commit/55c29a8774923f2d40fef7919b9490f61e57e7bb#diff-85c94ae15198235e2363f96216b9a1b2R565

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: pgindent && weirdness

Tom Lane-2
Alvaro Herrera <[hidden email]> writes:
> On 2020-Jan-16, Tom Lane wrote:
>> ... But I was hoping to
>> hear Piotr's opinion before moving forward.

> FWIW I think this code predates Piotr's involvement, I think; at least,
> it was already there in the FreeBSD code he imported:
> https://github.com/pstef/freebsd_indent/commit/55c29a8774923f2d40fef7919b9490f61e57e7bb#diff-85c94ae15198235e2363f96216b9a1b2R565

The roots of that code are even older than Postgres, I believe,
and there may not be anybody left who understands it completely.
But Piotr has certainly spent more time looking at it than I have,
so I'd still like to hear what he thinks.

                        regards, tom lane