Thread: 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
Attachment
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
Attachment
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
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