Thread: Lots of incorrect comments in nodeFuncs.c

Lots of incorrect comments in nodeFuncs.c

From
David Rowley
Date:
I noticed that nodeFuncs.c appears to have some pretty sloppy work
done in many of the comments.  Many look like they've just not been
updated from a copy/paste/edit from another node function.

The attached aims to clean these up.

I plan to push this a later today unless anyone has anything they'd
like to say about it first.

David

Attachment

Re: Lots of incorrect comments in nodeFuncs.c

From
Tom Lane
Date:
David Rowley <dgrowleyml@gmail.com> writes:
> I noticed that nodeFuncs.c appears to have some pretty sloppy work
> done in many of the comments.  Many look like they've just not been
> updated from a copy/paste/edit from another node function.
> The attached aims to clean these up.

I believe every one of these changes is wrong.
For instance:

         case T_ScalarArrayOpExpr:
-            coll = InvalidOid;    /* result is always boolean */
+            coll = InvalidOid;    /* result is always InvalidOid */
             break;

The point here is that the result type of ScalarArrayOpExpr is boolean,
which has no collation, therefore reporting its collation as InvalidOid
is correct.  Maybe there's a clearer way to say that, but your text is
more confusing not less so.

Likewise, the point of the annotations in exprSetCollation is to not
let a collation be applied to a node that must have a noncollatable
result type.

            regards, tom lane



Re: Lots of incorrect comments in nodeFuncs.c

From
David Rowley
Date:
On Fri, 9 Apr 2021 at 10:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > I noticed that nodeFuncs.c appears to have some pretty sloppy work
> > done in many of the comments.  Many look like they've just not been
> > updated from a copy/paste/edit from another node function.
> > The attached aims to clean these up.
>
> I believe every one of these changes is wrong.
> For instance:
>
>                 case T_ScalarArrayOpExpr:
> -                       coll = InvalidOid;      /* result is always boolean */
> +                       coll = InvalidOid;      /* result is always InvalidOid */
>                         break;
>
> The point here is that the result type of ScalarArrayOpExpr is boolean,
> which has no collation, therefore reporting its collation as InvalidOid
> is correct.  Maybe there's a clearer way to say that, but your text is
> more confusing not less so.

hmm ok.  I imagine there must be a better way to say that then since
it confused at least 1 reader so far.  My problem is that I assumed
"result" meant the result of the function that the comment is written
in, not the result of evaluating the given expression during
execution. If that was more clear then I'd not have been misled.

David



Re: Lots of incorrect comments in nodeFuncs.c

From
Tom Lane
Date:
David Rowley <dgrowleyml@gmail.com> writes:
> hmm ok.  I imagine there must be a better way to say that then since
> it confused at least 1 reader so far.  My problem is that I assumed
> "result" meant the result of the function that the comment is written
> in, not the result of evaluating the given expression during
> execution. If that was more clear then I'd not have been misled.

Maybe like

                case T_ScalarArrayOpExpr:
                    /* ScalarArrayOpExpr's result is boolean ... */
                    coll = InvalidOid;      /* ... so it has no collation */
                    break;

?

            regards, tom lane



Re: Lots of incorrect comments in nodeFuncs.c

From
Alvaro Herrera
Date:
On 2021-Apr-08, Tom Lane wrote:

> Maybe like
> 
>                 case T_ScalarArrayOpExpr:
>                     /* ScalarArrayOpExpr's result is boolean ... */
>                     coll = InvalidOid;      /* ... so it has no collation */
>                     break;

This is much clearer, yeah.

-- 
Álvaro Herrera       Valdivia, Chile



Re: Lots of incorrect comments in nodeFuncs.c

From
Michael Paquier
Date:
On Thu, Apr 08, 2021 at 09:21:30PM -0400, Alvaro Herrera wrote:
> On 2021-Apr-08, Tom Lane wrote:
>> Maybe like
>>
>>                 case T_ScalarArrayOpExpr:
>>                     /* ScalarArrayOpExpr's result is boolean ... */
>>                     coll = InvalidOid;      /* ... so it has no collation */
>>                     break;
>
> This is much clearer, yeah.

+1.
--
Michael

Attachment

Re: Lots of incorrect comments in nodeFuncs.c

From
David Rowley
Date:
On Fri, 9 Apr 2021 at 13:52, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Apr 08, 2021 at 09:21:30PM -0400, Alvaro Herrera wrote:
> > On 2021-Apr-08, Tom Lane wrote:
> >> Maybe like
> >>
> >>                 case T_ScalarArrayOpExpr:
> >>                     /* ScalarArrayOpExpr's result is boolean ... */
> >>                     coll = InvalidOid;      /* ... so it has no collation */
> >>                     break;
> >
> > This is much clearer, yeah.
>
> +1.

Yeah, that's much better.

For the exprSetCollation case, I ended up with:

                 case T_ScalarArrayOpExpr:
                     /* ScalarArrayOpExpr's result is boolean ... */
                     Assert(!OidIsValid(collation)); /* ... so never
set a collation */

I wanted something more like /* ... so we must never set a collation
*/ but that put the line longer than 80. I thought wrapping to a 2nd
line was excessive, so I shortened it to that.

David

Attachment

Re: Lots of incorrect comments in nodeFuncs.c

From
Tom Lane
Date:
David Rowley <dgrowleyml@gmail.com> writes:
> I wanted something more like /* ... so we must never set a collation
> */ but that put the line longer than 80. I thought wrapping to a 2nd
> line was excessive, so I shortened it to that.

LGTM.

            regards, tom lane



Re: Lots of incorrect comments in nodeFuncs.c

From
David Rowley
Date:
On Fri, 9 Apr 2021 at 23:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> LGTM.

Thanks. Pushed.

David