David Rowley <[email protected]> writes:
> Looking at the latest patch, I'd put this comment back to what I wrote:
> + * Return the running sum unchanged if the new input is null. This also
> + * covers the case where no non-null input has been seen yet, as the
> + * running sum is null then too.
> The extra sentence has an awful AI whiff to it.
The later comments could use more thought too. In particular,
I do not like this sort of pattern:
/* X is true. */
if (x)
// do something
To my mind it's more sensible as
if (x)
{
/* X is true. */
// do something
}
So in these bits:
+ /* This is the first non-null input. */
if (PG_ARGISNULL(0))
- {
- /* No non-null input seen so far... */
the replacement comment is badly placed.
At a less nit-picky level:
* In the avg_accum functions, we can argue about how likely it
is that we'd reach overflow of the "sum" fields, but it is completely
insane to expend cycles and code complexity to check for overflow of
the "count" fields. If you can reach 2^63 by repeated addition of 1
within the lifetime of a PG database, then we have got far worse
problems, eg with WAL LSN overflow.
* PG_RETURN_INPUT is not laid out per our usual conventions.
If you need a do/while wrapper, start it on the next line.
* I would not include one single one of these test cases.
They are not worth the development effort nor the forevermore
test runtime cost, especially since they are testing faked-up
scenarios.
regards, tom lane
