Обсуждение: Lots of incorrect comments in nodeFuncs.c

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

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

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

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

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

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

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

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

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

Thanks. Pushed.

David