On 14/01/2016 14:04, Geoff Winkless wrote:
> On 14 January 2016 at 11:19, Julien Rouhaud <julien.rouhaud@dalibo.com> wrote:
>> + /* don't try anything unless there's two Vars */
>> + if (varlist == NULL || list_length(varlist) < 2)
>> + continue;
>>
>> To be perfectly correct, the comment should say "at least two Vars".
>
> Apologies for butting in and I appreciate I don't have any ownership
> over this codebase or right to suggest any changes, but this just
> caught my eye before I could hit "delete".
>
> My mantra tends to be "why, not what" for inline comments; in this
> case you can get the same information from the next line of code as
> you get from the comment.
>
You're absolutely right, but in this case the comment is more like a
reminder of a bigger comment few lines before that wasn't quoted in my mail:
+ *[...] If there are no Vars then
+ * nothing need be done for this rel. If there's less than two Vars then
+ * there's no room to remove any, as the PRIMARY KEY must have at
least one
+ * Var, so we can safely also do nothing if there's less than two Vars.
so I assume it's ok to keep it this way.
> Perhaps something like
>
> /* it's clearly impossible to remove duplicates if there are fewer
> than two GROUPBY columns */
>
> might be more helpful?
>
> (also sorry if I've misunderstood what it _actually_ does, I just made
> an assumption based on reading this thread!)
>
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
