Coding style question

Shouldn't we turn on warnings by the compiler on uninitialized
variables? This can also be helpful.

--Imad
www.EnterpriseDB.com

<korryd@enterprisedb.com> writes:

I would probably write that as:

________________________________________________________________________

static TransactionId
_bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
Buffer buf, ScanKey itup_scankey)
{
TupleDesc itupdesc = RelationGetDescr(rel);
int natts = rel->rd_rel->relnatts;
Page page = BufferGetPage(buf);
OffsetNumber maxoff = PageGetMaxOffsetNumber(page);
BTPageOpaque opaque = (BTPageOpaque) PageGetSpecialPointer(page);
OffsetNumber offset = _bt_binsrch(rel, buf, natts, itup_scankey, false);
Buffer nbuf = InvalidBuffer;

The disadvantage of using initializers is that you end up contorting the code
to allow you to squeeze things into the initializers and it limits what you
can do later to the code without undoing them.

For example, if later you find out you have to, say, lock a table before the
itupdesc initializer then all of the sudden you have to rip out all the
initializers and rewrite them as assignments after the statement acquiring the
table lock.

I admit to a certain affinity to lisp-style programming and that does lead to
me tending to try to use initializers doing lots of work in expressions. But I
find I usually end up undoing them before I'm finished because I need to
include a statement or because too much work needs to be done with one
variable before some other variable can be initialized.

But including complex expensive function calls in initializers will probably
only confuse people trying to follow the logic of the code. Including
_bt_binsrch() as an initializer for example hides the bulk of the work this
function does.

People expect initializers to be simple expressions, macro calls, accessor
functions, and so on. Not to call out to complex functions that implement key
bits of the function behaviour.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

On Thu, 2006-11-02 at 23:53 +0500, imad wrote:

Shouldn't we turn on warnings by the compiler on uninitialized
variables? This can also be helpful.

Those warnings should already be enabled, at least with GCC.

-Neil

Gregory Stark wrote:

<korryd@enterprisedb.com> writes:

I would probably write that as:

________________________________________________________________________

static TransactionId
_bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
Buffer buf, ScanKey itup_scankey)
{
TupleDesc itupdesc = RelationGetDescr(rel);
int natts = rel->rd_rel->relnatts;
Page page = BufferGetPage(buf);
OffsetNumber maxoff = PageGetMaxOffsetNumber(page);
BTPageOpaque opaque = (BTPageOpaque) PageGetSpecialPointer(page);
OffsetNumber offset = _bt_binsrch(rel, buf, natts, itup_scankey, false);
Buffer nbuf = InvalidBuffer;

The disadvantage of using initializers is that you end up contorting the code
to allow you to squeeze things into the initializers and it limits what you
can do later to the code without undoing them.

True. And in any case, we tend not to be terrribly anal about style
preferences, especially if they are not documented.

I am sure I have done lots of things in ways other people would not
dream of, and I have certainly seen code done in a style I would never use.

This is a not atypical situation for open source projects, unlike
commercial situations where it is easier to enforce a corporate style.

cheers

andrew

On 11/2/06, Gregory Stark <stark@enterprisedb.com> wrote:

<korryd@enterprisedb.com> writes:

I would probably write that as:

________________________________________________________________________

static TransactionId
_bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
Buffer buf, ScanKey itup_scankey)
{
TupleDesc itupdesc = RelationGetDescr(rel);
int natts = rel->rd_rel->relnatts;
Page page = BufferGetPage(buf);
OffsetNumber maxoff = PageGetMaxOffsetNumber(page);
BTPageOpaque opaque = (BTPageOpaque) PageGetSpecialPointer(page);
OffsetNumber offset = _bt_binsrch(rel, buf, natts, itup_scankey, false);
Buffer nbuf = InvalidBuffer;

The disadvantage of using initializers is that you end up contorting the code
to allow you to squeeze things into the initializers and it limits what you
can do later to the code without undoing them.

For example, if later you find out you have to, say, lock a table before the
itupdesc initializer then all of the sudden you have to rip out all the
initializers and rewrite them as assignments after the statement acquiring the
table lock.

Well, its about the coding style. And I doubt there exists a data type
which may not have
an initializer. A NULL / Zero is an option in all cases and you can do
whatever you want to assign it a value immediately after the
initialization section. My two cents!

--Imad
www.EnterpriseDB.com

The disadvantage of using initializers is that you end up contorting the code
to allow you to squeeze things into the initializers and it limits what you
can do later to the code without undoing them.

For example, if later you find out you have to, say, lock a table before the
itupdesc initializer then all of the sudden you have to rip out all the
initializers and rewrite them as assignments after the statement acquiring the
table lock.

Good point (and I can't argue with your example). But, I think
initializers also force you to declare variables in the scope where they
are needed. Instead of declaring every variable at the start of the
function, it's better to declare them as nested as practical (not as
nested as possible, but as nested as practical). That means, you might
write the code like this:

static TransactionId
_bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,
Buffer buf, ScanKey itup_scankey)
{
if( lockTable( ... ))
{
TupleDesc itupdesc = RelationGetDescr(rel);
int natts = rel->rd_rel->relnatts;
Page page = BufferGetPage(buf);
OffsetNumber maxoff = PageGetMaxOffsetNumber(page);
...

The biggest advantage to that style of coding is that you know when each
variable goes out of scope. If you declare every variable at the start
of the function (and you don't initialize it), it can be very difficult
to tell at which points in the code the variable holds a (meaningful)
value. If you declare local variables in nested scopes, you know that
the variable disappears as soon as you see the closing '}'.

I admit to a certain affinity to lisp-style programming and that does lead to
me tending to try to use initializers doing lots of work in expressions. But I
find I usually end up undoing them before I'm finished because I need to
include a statement or because too much work needs to be done with one
variable before some other variable can be initialized.

But including complex expensive function calls in initializers will probably
only confuse people trying to follow the logic of the code. Including
_bt_binsrch() as an initializer for example hides the bulk of the work this
function does.

People expect initializers to be simple expressions, macro calls, accessor
functions, and so on. Not to call out to complex functions that implement key
bits of the function behaviour.

Agreed - you can certainly take initialization way too far, I just think
we don't take it far enough in many cases and I'm wondering if there's
some advantage that I'm not aware of.

-- Korry

Gregory Stark <stark@enterprisedb.com> writes:

People expect initializers to be simple expressions, macro calls, accessor
functions, and so on. Not to call out to complex functions that implement key
bits of the function behaviour.

Yeah, I agree with that. But as Andrew noted, we don't really have any
hard and fast coding rules --- the only guideline is to do your best to
make your code readable, because other people *will* have to read it.

In the particular example here I find Korry's proposed coding less
readable than what's there, but I can't entirely put my finger on why.
Maybe it's just the knowledge that it's less easily modifiable. In general,
I'd say initializers with side effects or nonobvious ordering dependencies
are definitely bad style, because someone might innocently rearrange
them, eg to group all the variables of the same datatype together.
You can get away with ordering dependencies like

TupleDesc itupdesc = RelationGetDescr(rel);
int natts = itupdesc->natts;

because the dependency is obvious (even to the compiler). Anything more
complex than this, I'd write as a statement not an initializer.

regards, tom lane

Shouldn't we turn on warnings by the compiler on uninitialized
variables? This can also be helpful.

Those warnings should already be enabled, at least with GCC.

Yes, the compiler can detect unitialized variables,

But, that introduces a new problem. There are a lot of tools out there
(like GCC) that can find unitialized variables (or more specifically,
variables which are used before initialization). I've seen too many
less-scarred developers add an " = NULL" to quiet down the tool. But
that's (arguably) worse than leaving the variable uninitialized - if you
simply initialize a variable to a known (but not correct) value, you've
disabled the tool; it can't help you find improperly initialized
variables anymore. The variable has a value, but you still don't know
at which point in time it has a correct value.

That's another reason I prefer initializers (and nested variable
declarations) - I can put off creating the variable until I can assign a
meaningful value to it. (I don't go so far as to introduce artificial
scopes just for the sake of nesting variable declarations).

Too many scars...

-- Korry

imad <immaad@gmail.com> writes:

Well, its about the coding style. And I doubt there exists a data type
which may not have
an initializer. A NULL / Zero is an option in all cases and you can do
whatever you want to assign it a value immediately after the
initialization section. My two cents!

Actually, there are a lot of situations where putting an initializer is
definitely *bad* style in my opinion, because it can hide errors of
omission that the compiler would otherwise find for you. The most
common example you'll see in the Postgres code is variables that should
be set in each branch of an if or switch construct:

int val;

switch (foo)
{
case A:
val = 42;
break;
case B:
val = 52;
break;
...
default:
elog(ERROR, ...);
val = 0; /* keep compiler quiet */
break;
}

return val;

Someone might think it better to initialize val to 0 and get rid of the
useless (unreachable) assignment in the default case, but I say not.
With this structure, you'll get an uninitialized-variable warning if
you forget to set "val" in any one of the cases of the switch. That's
a check worth having, especially if the code is at all complicated
within the cases.

When the variable is going to be set anyway in straight-line code at the
top of the function, then it's mostly a matter of taste whether you set
it with an initializer or an assignment. But whenever there are
multiple places that might need to set it, I try to structure the code
to exploit the compiler's ability to catch uninitialized variables,
and that means not using an initializer.

regards, tom lane

Yeah, I agree with that. But as Andrew noted, we don't really have any
hard and fast coding rules --- the only guideline is to do your best to
make your code readable, because other people *will* have to read it.

I'm not really looking for hard/fast rules. Just picking brains.

In the particular example here I find Korry's proposed coding less
readable than what's there, but I can't entirely put my finger on why.
Maybe it's just the knowledge that it's less easily modifiable. In general,
I'd say initializers with side effects or nonobvious ordering dependencies
are definitely bad style, because someone might innocently rearrange
them, eg to group all the variables of the same datatype together.
You can get away with ordering dependencies like

TupleDesc itupdesc = RelationGetDescr(rel);
int natts = itupdesc->natts;

because the dependency is obvious (even to the compiler). Anything more
complex than this, I'd write as a statement not an initializer.

Agreed - I contrived an example (I just happened to be reading
_bt_check_unique()). In fact, I would not initialize 'offset' as I
suggested, but I probably would initialize just about everything else.

(In fact, in the actual code, there's a code-comment that describes the
initialization of offset - I would certainly leave that in place).

-- Korry

<korryd@enterprisedb.com> writes:

initializers also force you to declare variables in the scope where they
are needed. Instead of declaring every variable at the start of the
function, it's better to declare them as nested as practical (not as
nested as possible, but as nested as practical).

I agree that in many places it'd be better style to declare variables in
smaller scopes ... but that's not the point you started the thread with.
In any case, the initializer-vs-assignment decision is the same no
matter what scope you're talking about --- I don't see how that "forces"
you to do it either way.

regards, tom lane

On 11/3/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:

imad <immaad@gmail.com> writes:

Well, its about the coding style. And I doubt there exists a data type
which may not have
an initializer. A NULL / Zero is an option in all cases and you can do
whatever you want to assign it a value immediately after the
initialization section. My two cents!

Actually, there are a lot of situations where putting an initializer is
definitely *bad* style in my opinion, because it can hide errors of
omission that the compiler would otherwise find for you. The most
common example you'll see in the Postgres code is variables that should
be set in each branch of an if or switch construct:

int val;

switch (foo)
{
case A:
val = 42;
break;
case B:
val = 52;
break;
...
default:
elog(ERROR, ...);
val = 0; /* keep compiler quiet */
break;
}

return val;

Someone might think it better to initialize val to 0 and get rid of the
useless (unreachable) assignment in the default case, but I say not.
With this structure, you'll get an uninitialized-variable warning if
you forget to set "val" in any one of the cases of the switch. That's
a check worth having, especially if the code is at all complicated
within the cases.

When the variable is going to be set anyway in straight-line code at the
top of the function, then it's mostly a matter of taste whether you set
it with an initializer or an assignment. But whenever there are
multiple places that might need to set it, I try to structure the code
to exploit the compiler's ability to catch uninitialized variables,
and that means not using an initializer.

regards, tom lane

Thats right!
The next thing is that, should this be left out on the compiler? I
mean, there may still be some scenarios where compiler won't be able
to help us. For instance, passing an uninitialized variable to a
function by reference.

Regards
--Imad

On Thu, 2006-11-02 at 14:23 -0500, korryd@enterprisedb.com wrote:

Yes, the compiler can detect unitialized variables,

In most situations, anyway.

I've seen too many less-scarred developers add an " = NULL" to quiet
down the tool. But that's (arguably) worse than leaving the variable
uninitialized - if you simply initialize a variable to a known (but
not correct) value, you've disabled the tool; it can't help you find
improperly initialized variables anymore.

I definitely agree that this is not good style[1]http://advogato.org/person/nconway/diary.html?start=24 -- if you see any
Postgres code that does this, I think it should be fixed. (This is
probably something you could teach a compiler to warn you about, as
well.)

That's another reason I prefer initializers (and nested variable
declarations) - I can put off creating the variable until I can assign
a meaningful value to it.

Well, clearly you should only assign meaningful values to variables, but
I don't see anything wrong with omitting an initializer, initializing
the variable before using it, and letting the compiler warn you if you
forget to do this correctly. I agree with Greg's rationale on when to
include an initializer in a variable declaration (when the initializer
is trivial: e.g. casting a void * to a more specific pointer type, or
using one of the PG_GETARG_XXX() macros in a UDF).

(I don't go so far as to introduce artificial scopes just for the sake
of nesting variable declarations).

I don't introduce artificial scopes either. However, I do try to declare
variables in the most-tightly-enclosing scope. For example, if a
variable is only used in one branch of an if statement, declare the
variable inside that block, not in the enclosing scope.

I also find that if you're declaring a lot of variables in a single
block, that's usually a sign that the block is too large and should be
refactored (e.g. by moving some code into separate functions). If you
keep your functions manageably small (which is not always the case in
the Postgres code, unfortunately), the declarations are usually pretty
clearly visible.

-Neil

[1]: http://advogato.org/person/nconway/diary.html?start=24

<korryd@enterprisedb.com> writes:

initializers also force you to declare variables in the scope where they
are needed. Instead of declaring every variable at the start of the
function, it's better to declare them as nested as practical (not as
nested as possible, but as nested as practical).

I agree that in many places it'd be better style to declare variables in
smaller scopes ... but that's not the point you started the thread with.
In any case, the initializer-vs-assignment decision is the same no
matter what scope you're talking about --- I don't see how that "forces"
you to do it either way.

Right - I should have said that proper initialization encourages you to
declare variables in nested scopes (proper meaning that the initializer
puts a meaningful value into the variable, not just a default NULL or 0)
- if the initializer depends on a computed value, you can't initialize
until that value has been computed.

I guess the two issues are not all that related - you can initialize
without nesting (in many cases) and you can nest without initializing.
They are both readability and maintainability issues to me.

Thanks for the feedback.

-- Korry

Well, clearly you should only assign meaningful values to variables, but
I don't see anything wrong with omitting an initializer, initializing
the variable before using it, and letting the compiler warn you if you
forget to do this correctly.

The problem that that introduces is that you have to unravel the code if
you want to maintain it, especially if you're changing complex code
(many code paths) and some variable is initialized long after it's
declared. You have to search the code to figure out at which point the
variable gains a meaningful value. In the example I cited, each
variable was assigned a value immediately after being declared. That
wasn't a good example - in some places, we declare all variables at the
start of the function, but we don't assign a value to some of the
variables until 20 or 30 lines of code later (and if there are multiple
code paths, you have to follow each one to find out when the variable
gains a meaningful value).

I agree with Greg's rationale on when to
include an initializer in a variable declaration (when the initializer
is trivial: e.g. casting a void * to a more specific pointer type, or
using one of the PG_GETARG_XXX() macros in a UDF).

I agree too. I wasn't trying to suggest that every variable must be
initialized.

I think Tom stated it pretty well:

When the variable is going to be set anyway in straight-line
code at the top of the function, then it's mostly a matter of
taste whether you set it with an initializer or an assignment.

the key phrase is: "set anyway in straigh-tline code at the top of the
function"

(I don't go so far as to introduce artificial scopes just for the sake
of nesting variable declarations).

I don't introduce artificial scopes either. However, I do try to declare
variables in the most-tightly-enclosing scope. For example, if a
variable is only used in one branch of an if statement, declare the
variable inside that block, not in the enclosing scope.

good...

I also find that if you're declaring a lot of variables in a single
block, that's usually a sign that the block is too large and should be
refactored (e.g. by moving some code into separate functions). If you
keep your functions manageably small (which is not always the case in
the Postgres code, unfortunately), the declarations are usually pretty
clearly visible.

I couldn't agree more.

Thanks for your comments.

-- Korry

I think Tom stated it pretty well:

When the variable is going to be set anyway in straight-line code
at the top of the function, then it's mostly a matter of taste
whether you set it with an initializer or an assignment.

the key phrase is: "set anyway in straigh-tline code /at the top of
the function//_"_/

(I don't go so far as to introduce artificial scopes just for the sake
of nesting variable declarations).

I don't introduce artificial scopes either. However, I do try to declare
variables in the most-tightly-enclosing scope. For example, if a
variable is only used in one branch of an if statement, declare the
variable inside that block, not in the enclosing scope.

good...

This may not inform the current conversation at all, but a while back I
went on a cross-compiler compatibility binge for all of my active
projects, and I found that some compilers (*cough* Borland *cough) had
some very strange compiler/run time errors unless all variables were
declared at the top of the function, before any other code gets
executed. For better or for worse, I started strictly declaring all
variables in this manner, with initialization happening afterward, and
the behavior has stuck with me. I don't know whether any compilers used
for postgres builds still have this issue - it's been a few years.

I also find that if you're declaring a lot of variables in a single
block, that's usually a sign that the block is too large and should be
refactored (e.g. by moving some code into separate functions). If you
keep your functions manageably small (which is not always the case in
the Postgres code, unfortunately), the declarations are usually pretty
clearly visible.

I couldn't agree more.

Insert emphatic agreement here. Refactoring into smaller functions or
doing a bit of object orientation almost always solves that readability
problem for me.

--
Nolan Cafferky
Software Developer
IT Department
RBS Interactive
nolan.cafferky@rbsinteractive.com

Nolan Cafferky wrote:

This may not inform the current conversation at all, but a while back
I went on a cross-compiler compatibility binge for all of my active
projects, and I found that some compilers (*cough* Borland *cough) had
some very strange compiler/run time errors unless all variables were
declared at the top of the function, before any other code gets
executed. For better or for worse, I started strictly declaring all
variables in this manner, with initialization happening afterward, and
the behavior has stuck with me. I don't know whether any compilers
used for postgres builds still have this issue - it's been a few years.

We expect the compiler to be C89 compliant at a minimum. If it rejects
simple initialisation in the declarations or variables declared in an
inner scope then it's hopeless for our purposes, surely. We have lots of
such code.

cheers

andrew