On Tue, Nov 17, 2015 at 7:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think this might do the wrong thing with block numbers above 0x80000000
> and/or offset numbers above 0x8000. I'd be more comfortable about it if
> + encoded = ((int64) block << 16) | offset;
> were
> + encoded = ((uint64) block << 16) | (uint16) offset;
> so that there's no risk of the compiler deciding to sign-extend rather
> than zero-fill either input.
I don't have a problem with your alternative, but I don't see any risk
with the original. It is recommended by various coding standards to
only use bitwise operators on unsigned operands, so that's a good
enough reason, I suppose.
> Also, I think it'd be a good idea to explicitly set indexcursor = NULL
> in the tuplesort_empty case; the previous coding was effectively doing
> that. It's true that the code shouldn't attempt to touch the value,
> but it's better not to have dangling pointers lying around.
The code started that way, but I removed the "indexcursor = NULL"
because the previous coding was *not* doing that. tuplesort_getdatum()
was not setting the passed Datum pointer (which points to indexcursor
here) in the event of returning false. Maybe I should have left in the
code setting indexcursor = NULL all the same.
--
Peter Geoghegan
