ScanDirections
Small patch that makes ScanDirection a char(f|b|n) instead of a int or long or
whatever enum makes it. I tried to track down all the areas that depend on it
being specifically -1, 0, or 1. heapam appeared to be the biggest/only one.
(At least `gmake check` passed.)
--
Regards, James William Pye
Attachments:
scandir.patchtext/plain; charset=utf-8Download+86-87
On Sun, 2006-02-19 at 17:47 -0700, James William Pye wrote:
Small patch that makes ScanDirection a char(f|b|n) instead of a int or long or
whatever enum makes it.
Why?
-Neil
On Sun, Feb 19, 2006 at 08:02:09PM -0500, Neil Conway wrote:
Why?
*Very* minor. one byte instead of four.
Yes, not really worth the time, but I was poking around at code anyways and
decided to write up a little patch.
--
Regards, James William Pye
James William Pye wrote:
On Sun, Feb 19, 2006 at 08:02:09PM -0500, Neil Conway wrote:
Why?
*Very* minor. one byte instead of four.
Is one byte any faster anyway?
I would be expecting that with 64bit PC's a 64bit item is as fast, if
not faster than a 32bit, or 8 bit.
Show quoted text
Yes, not really worth the time, but I was poking around at code anyways and
decided to write up a little patch.
On Sun, 2006-02-19 at 18:12 -0700, James William Pye wrote:
*Very* minor. one byte instead of four.
Arguably, enumerations are more readable than #defines and easier to
debug. Why do we care about saving 3 bytes for ScanDirection?
-Neil
On Sun, Feb 19, 2006 at 08:21:31PM -0500, Neil Conway wrote:
Arguably, enumerations are more readable than #defines and easier to
debug.
Agreed. However, I didn't think it would impede much here as I figured gdb would
display 'f' or 'b' or 'n', dunno for sure tho as I didn't try.
Why do we care about saving 3 bytes for ScanDirection?
It's a drop in the bucket--or maybe water vapor in the vacinity, so I doubt I
could find a stunning reason.
--
Regards, James William Pye
James William Pye <pgsql@jwp.name> writes:
Small patch that makes ScanDirection a char(f|b|n) instead of a int or
long or whatever enum makes it.
Why is this a good idea? It strikes me as likely to break things,
without really buying anything.
regards, tom lane
On Sun, Feb 19, 2006 at 09:04:09PM -0500, Tom Lane wrote:
James William Pye <pgsql@jwp.name> writes:
Small patch that makes ScanDirection a char(f|b|n) instead of a int or
long or whatever enum makes it.Why is this a good idea?
A more appropriately sized variable for the data that it will be holding?
Certainly not a stunning improvement considering that it's only three bytes.
It strikes me as likely to break things, without really buying anything.
I tried to be careful and track everything down. However, I imagine I could
have easily missed something, so I understand that concern.
In any case, some parts of the patch merely makes some code make use of the
sdir.h macros instead of depending on -1/0/1--heapam in particular. At least
those portions should be applied, no?
--
Regards, James William Pye
James William Pye <pgsql@jwp.name> writes:
In any case, some parts of the patch merely makes some code make use of the
sdir.h macros instead of depending on -1/0/1--heapam in particular. At least
those portions should be applied, no?
I can't see any great advantage there either ...
regards, tom lane
On Sun, 2006-02-19 at 19:46 -0700, James William Pye wrote:
In any case, some parts of the patch merely makes some code make use of the
sdir.h macros instead of depending on -1/0/1--heapam in particular. At least
those portions should be applied, no?
I agree: using the symbolic names for the ScanDirection alternatives is
more readable than using magic numbers, and costs us nothing.
-Neil
James William Pye wrote:
In any case, some parts of the patch merely makes some code make use of the
sdir.h macros instead of depending on -1/0/1--heapam in particular. At least
those portions should be applied, no?
I've applied the attached patch to CVS HEAD that just changes the code
to use the sdir.h macros when appropriate. Thanks for the patch.
-Neil