On Wed, Mar 25, 2015 at 6:26 PM, Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
>>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes:
>
> Peter> You still pointlessly check memtupcount here:
>
> Peter> + if (memtupcount < 10000 || nss->input_count < 10000 || !nss->estimating)
> Peter> + return false;
>
> It's in a register; the test is free.
Unbelievable! The test is "free", and you wrote it that way in the
first place, so we should just leave it that way rather than changing
it to suit you?
As things stand, the test is totally misleading. In fact, I think it
was the source of my initial confusion about the way you track
non-null tuple count separately. I certainly didn't have any clue from
comments. Apart from fixing this, I added some comments explaining
that in my version, but you didn't make any effort to follow that
either.
> Peter> This cast to void is unnecessary:
>
> Peter> + (void) ssup;
>
> It's an explicit statement that the parameter is otherwise unused.
> Maybe that compiler warning isn't usually on by default, but I
> personally regard it as good style to be explicit about it.
Your personal preferences don't enter into it, since every sort
support routine since 2011 has followed that style (most still don't
touch the sortsupport object). That's the way things work around here
- consistency matters. If the existing convention is wrong, we fix the
existing convention.
Why do you get to ignore well established practice like this? Are you special?
> Peter> Please try and at least consider my feedback. I don't expect you
> Peter> to do exactly what I ask, but I also don't expect you to
> Peter> blithely ignore it.
>
> You should really stop digging this hole deeper.
You have that backwards.
> >> The INT64_MIN/MAX changes should be committed fairly soon. (I
> >> haven't posted a patch for TRACE_SORT)
>
> Peter> I wouldn't assume that.
>
> Oh ye of little faith. I would not have said that had I not already been
> informed of it by a committer, and indeed it is now committed.
You could have said so.
--
Peter Geoghegan
