Thread: Coding style question
I've noticed a trend in the PostgreSQL code base - for some reason, we tend to avoid initializing automatic variables
(actually,the code base is pretty mixed on this point).<br /><br /> For example in _bt_check_unique() we have:<br /><hr
noshade="NOSHADE"/><pre>
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;
OffsetNumber offset,
maxoff;
Page page;
BTPageOpaque opaque;
Buffer nbuf = InvalidBuffer;
page = BufferGetPage(buf);
opaque = (BTPageOpaque) PageGetSpecialPointer(page);
maxoff = PageGetMaxOffsetNumber(page);
offset = _bt_binsrch(rel, buf, natts, itup_scankey, false);...
</pre><hr noshade="NOSHADE" /><br /><br /> Notice that four variables are not initialized; instead we assign values to
themimmediately after declaring them. <br /><br /> I would probably write that as:<br /><hr noshade="NOSHADE" /><pre>
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;...
</pre><hr noshade="NOSHADE" /><br /><br /> I'm not trying to be pedantic (it just comes naturally), but is there some
reasonthat the first form would be better? I know that there is no difference in the resulting code, so this is purely
astyle/maintainability question.<br /><br /> I guess the first form let's you intersperse comments (which is good).
<br/><br /> On the other hand, the second form makes it easy to see which variables are un-initialized. The second
formalso prevents you from adding any code between declaring the variable and assigning a value to it (which is good in
complicatedcode; you might introduce a reference to an unitialized variable).<br /><br /> Just curious.<br /><br />
-- Korry
Shouldn't we turn on warnings by the compiler on uninitialized
variables? This can also be helpful.
--Imad
www.EnterpriseDB.com
On 11/2/06, korryd@enterprisedb.com <korryd@enterprisedb.com> wrote:
>
> I've noticed a trend in the PostgreSQL code base - for some reason, we tend
> to avoid initializing automatic variables (actually, the code base is pretty
> mixed on this point).
>
> For example in _bt_check_unique() we have:
> ________________________________
> 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;
> OffsetNumber offset,
> maxoff;
> Page page;
> BTPageOpaque opaque;
> Buffer nbuf = InvalidBuffer;
>
> page = BufferGetPage(buf);
> opaque = (BTPageOpaque) PageGetSpecialPointer(page);
> maxoff = PageGetMaxOffsetNumber(page);
> offset = _bt_binsrch(rel, buf, natts, itup_scankey, false);
> ...
>
>
> ________________________________
>
>
> Notice that four variables are not initialized; instead we assign values to
> them immediately after declaring them.
>
> 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;
> ...
>
> ________________________________
>
>
> I'm not trying to be pedantic (it just comes naturally), but is there some
> reason that the first form would be better? I know that there is no
> difference in the resulting code, so this is purely a style/maintainability
> question.
>
> I guess the first form let's you intersperse comments (which is good).
>
> On the other hand, the second form makes it easy to see which variables are
> un-initialized. The second form also prevents you from adding any code
> between declaring the variable and assigning a value to it (which is good in
> complicated code; you might introduce a reference to an unitialized
> variable).
>
> Just curious.
>
> -- Korry
<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
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
<br /><blockquote type="CITE"><pre>
<font color="#000000">The disadvantage of using initializers is that you end up contorting the code</font>
<font color="#000000">to allow you to squeeze things into the initializers and it limits what you</font>
<font color="#000000">can do later to the code without undoing them.</font>
<font color="#000000">For example, if later you find out you have to, say, lock a table before the</font>
<font color="#000000">itupdesc initializer then all of the sudden you have to rip out all the</font>
<font color="#000000">initializers and rewrite them as assignments after the statement acquiring the</font>
<font color="#000000">table lock.</font>
</pre></blockquote><br /> Good point (and I can't argue with your example). But, I think initializers also force you
todeclare variables in the scope where they are needed. Instead of declaring <i>every</i> 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).
Thatmeans, you might write the code like this:<br /><br /><pre>
<tt><font color="#000000">static TransactionId</font></tt>
<tt><font color="#000000">_bt_check_unique(Relation rel, IndexTuple itup, Relation heapRel,</font></tt>
<tt><font color="#000000"> Buffer buf, ScanKey itup_scankey)</font></tt>
<tt><font color="#000000">{</font></tt><font color="#000000"> </font>
<font color="#000000"> if( lockTable( ... ))</font>
<font color="#000000"> {</font>
<tt><font color="#000000"> TupleDesc itupdesc = RelationGetDescr(rel);</font></tt>
<tt><font color="#000000"> int natts = rel->rd_rel->relnatts;</font></tt>
<tt><font color="#000000"> Page page = BufferGetPage(buf);</font></tt>
<tt><font color="#000000"> OffsetNumber maxoff = PageGetMaxOffsetNumber(page);</font></tt>
<font color="#000000"> ...</font>
</pre> The biggest advantage to that style of coding is that you <i>know</i> when each variable goes out of scope. If
youdeclare every variable at the start of the function (and you don't initialize it), it can be very difficult to tell
atwhich points in the code the variable holds a (meaningful) value. If you declare local variables in nested scopes,
youknow that the variable disappears as soon as you see the closing '}'.<br /><br /><blockquote type="CITE"><pre>
<font color="#000000">I admit to a certain affinity to lisp-style programming and that does lead to</font>
<font color="#000000">me tending to try to use initializers doing lots of work in expressions. But I</font>
<font color="#000000">find I usually end up undoing them before I'm finished because I need to</font>
<font color="#000000">include a statement or because too much work needs to be done with one</font>
<font color="#000000">variable before some other variable can be initialized.</font>
<font color="#000000">But including complex expensive function calls in initializers will probably</font>
<font color="#000000">only confuse people trying to follow the logic of the code. Including</font>
<font color="#000000">_bt_binsrch() as an initializer for example hides the bulk of the work this</font>
<font color="#000000">function does.</font>
<font color="#000000">People expect initializers to be simple expressions, macro calls, accessor</font>
<font color="#000000">functions, and so on. Not to call out to complex functions that implement key</font>
<font color="#000000">bits of the function behaviour.</font>
</pre></blockquote><br /> Agreed - you can certainly take initialization way too far, I just think we don't take it far
enoughin many cases and I'm wondering if there's some advantage that I'm not aware of.<br /><br /> --
Korry<br/><br />
<blockquote type="CITE"><pre> <font color="#000000">> Shouldn't we turn on warnings by the compiler on uninitialized</font> <font color="#000000">> variables? This can also be helpful.</font> <font color="#000000">Those warnings should already be enabled, at least with GCC.</font> </pre></blockquote><br /> Yes, the compiler can detect unitialized variables, <br /><br /> But, that introduces a new problem. There are a lot of tools out there (like GCC) that can find unitialized variables (or more specifically, variableswhich are used before initialization). I've seen too many less-scarred developers add an " = NULL" to quiet downthe tool. But that's (arguably) worse than leaving the variable uninitialized - if you simply initialize a variableto a known (but not correct) value, you've disabled the tool; it can't help you find improperly initialized variablesanymore. The variable has a value, but you still don't know at which point in time it has a <i>correct</i> value.<br/><br /> That's another reason I prefer initializers (and nested variable declarations) - I can put off creatingthe variable until I can assign a meaningful value to it. (I don't go so far as to introduce <i>artificial</i>scopes just for the sake of nesting variable declarations).<br /><br /> Too many scars...<br /><br /> -- Korry<br /><br />
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
<br /><blockquote type="CITE"><pre> <font color="#000000">Yeah, I agree with that. But as Andrew noted, we don't really have any</font> <font color="#000000">hard and fast coding rules --- the only guideline is to do your best to</font> <font color="#000000">make your code readable, because other people *will* have to read it.</font> </pre></blockquote><br /> I'm not really looking for hard/fast rules. Just picking brains. <br /><br /><blockquote type="CITE"><pre> <font color="#000000">In the particular example here I find Korry's proposed coding less</font> <font color="#000000">readable than what's there, but I can't entirely put my finger on why.</font> <font color="#000000">Maybe it's just the knowledge that it's less easily modifiable. In general,</font> <font color="#000000">I'd say initializers with side effects or nonobvious ordering dependencies</font> <font color="#000000">are definitely bad style, because someone might innocently rearrange</font> <font color="#000000">them, eg to group all the variables of the same datatype together.</font> <font color="#000000">You can get away with ordering dependencies like</font> <font color="#000000"> TupleDesc itupdesc = RelationGetDescr(rel);</font> <font color="#000000"> int natts = itupdesc->natts;</font> <font color="#000000">because the dependency is obvious (even to the compiler). Anything more</font> <font color="#000000">complex than this, I'd write as a statement not an initializer.</font> </pre></blockquote><br /> Agreed - I contrived an example (I just happened to be reading _bt_check_unique()). In fact, Iwould <i>not</i> initialize 'offset' as I suggested, but I probably would initialize just about everything else.<br /><br/> (In fact, in the actual code, there's a code-comment that describes the initialization of offset - I would certainlyleave that in place).<br /><br /> -- Korry<br /><br /><br />
<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] -- 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
<blockquote type="CITE"><pre> <font color="#000000"><<a href="mailto:korryd@enterprisedb.com">korryd@enterprisedb.com</a>> writes:</font> <font color="#000000">> initializers also force you to declare variables in the scope where they</font> <font color="#000000">> are needed. Instead of declaring every variable at the start of the</font> <font color="#000000">> function, it's better to declare them as nested as practical (not as</font> <font color="#000000">> nested as possible, but as nested as practical).</font> <font color="#000000">I agree that in many places it'd be better style to declare variables in</font> <font color="#000000">smaller scopes ... but that's not the point you started the thread with.</font> <font color="#000000">In any case, the initializer-vs-assignment decision is the same no</font> <font color="#000000">matter what scope you're talking about --- I don't see how that "forces"</font> <font color="#000000">you to do it either way.</font> </pre></blockquote><br /> Right - I should have said that <i>proper</i> initialization encourages you to declare variablesin nested scopes (<i>proper</i> meaning that the initializer puts a meaningful value into the variable, not justa default NULL or 0) - if the initializer depends on a computed value, you can't initialize until that value has beencomputed. <br /><br /> I guess the two issues are not all that related - you can initialize without nesting (in manycases) and you can nest without initializing. They are both readability and maintainability issues to me.<br /><br />Thanks for the feedback. <br /><br /> -- Korry<br /><br /><br />
<br /><blockquote type="CITE"><pre> <font color="#000000">Well, clearly you should only assign meaningful values to variables, but</font> <font color="#000000">I don't see anything wrong with omitting an initializer, initializing</font> <font color="#000000">the variable before using it, and letting the compiler warn you if you</font> <font color="#000000">forget to do this correctly. </font> </pre></blockquote><br /> 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 exampleI cited, each variable was assigned a value immediately after being declared. That wasn't a good example - in someplaces, we declare all variables at the start of the function, but we don't assign a value to some of the variables until20 or 30 lines of code later (and if there are multiple code paths, you have to follow each one to find out when thevariable gains a meaningful value). <br /><br /><blockquote type="CITE"><pre> <font color="#000000">I agree with Greg's rationale on when to</font> <font color="#000000">include an initializer in a variable declaration (when the initializer</font> <font color="#000000">is trivial: e.g. casting a void * to a more specific pointer type, or</font> <font color="#000000">using one of the PG_GETARG_XXX() macros in a UDF).</font> </pre></blockquote><br /> I agree too. I wasn't trying to suggest that <i>every variable must be initialized. </i><br /><br/> I think Tom stated it pretty well:<br /><blockquote><tt>When the variable is going to be set anyway in straight-linecode at the top of the function, then it's mostly a matter of taste whether you set it with an initializer oran assignment.</tt><br /></blockquote> the key phrase is: "set anyway in straigh-tline code <i>at the top of the function</i><i><u>"</u></i><blockquotetype="CITE"><pre> <font color="#000000">> (I don't go so far as to introduce artificial scopes just for the sake</font> <font color="#000000">> of nesting variable declarations).</font> <font color="#000000">I don't introduce artificial scopes either. However, I do try to declare</font> <font color="#000000">variables in the most-tightly-enclosing scope. For example, if a</font> <font color="#000000">variable is only used in one branch of an if statement, declare the</font> <font color="#000000">variable inside that block, not in the enclosing scope.</font> </pre></blockquote> good... <blockquote type="CITE"><pre> <font color="#000000">I also find that if you're declaring a lot of variables in a single</font> <font color="#000000">block, that's usually a sign that the block is too large and should be</font> <font color="#000000">refactored (e.g. by moving some code into separate functions). If you</font> <font color="#000000">keep your functions manageably small (which is not always the case in</font> <font color="#000000">the Postgres code, unfortunately), the declarations are usually pretty</font> <font color="#000000">clearly visible.</font> </pre></blockquote><br /> I couldn't agree more.<br /><br /> Thanks for your comments.<br /><br /> -- Korry<br/><br />
<br /><blockquote cite="mid1162498576.7998.324.camel@sakai.localdomain" type="cite">I think Tom stated it pretty well:<br/><blockquote><tt>When the variable is going to be set anyway in straight-line code at the top of the function, thenit's mostly a matter of taste whether you set it with an initializer or an assignment.</tt><br /></blockquote> the keyphrase is: "set anyway in straigh-tline code <i>at the top of the function</i><i><u>"</u></i><blockquote type="CITE"><pre><fontcolor="#000000">> (I don't go so far as to introduce artificial scopes just for the sake</font> <font color="#000000">> of nesting variable declarations).</font> <font color="#000000">I don't introduce artificial scopes either. However, I do try to declare</font> <font color="#000000">variables in the most-tightly-enclosing scope. For example, if a</font> <font color="#000000">variable is only used in one branch of an if statement, declare the</font> <font color="#000000">variable inside that block, not in the enclosing scope.</font> </pre></blockquote> good...</blockquote>This may not inform the current conversation at all, but a while back I went on a cross-compiler compatibilitybinge for all of my active projects, and I found that some compilers (*cough* Borland *cough) had some verystrange compiler/run time errors unless all variables were declared at the top of the function, before any other codegets executed. For better or for worse, I started strictly declaring all variables in this manner, with initializationhappening afterward, and the behavior has stuck with me. I don't know whether any compilers used for postgresbuilds still have this issue - it's been a few years.<br /><blockquote cite="mid1162498576.7998.324.camel@sakai.localdomain"type="cite"><blockquote type="CITE"><pre><font color="#000000">I alsofind that if you're declaring a lot of variables in a single</font> <font color="#000000">block, that's usually a sign that the block is too large and should be</font> <font color="#000000">refactored (e.g. by moving some code into separate functions). If you</font> <font color="#000000">keep your functions manageably small (which is not always the case in</font> <font color="#000000">the Postgres code, unfortunately), the declarations are usually pretty</font> <font color="#000000">clearly visible.</font> </pre></blockquote><br /> I couldn't agree more.<br /><br /></blockquote>Insert emphatic agreement here. Refactoring into smaller functions or doing a bit of object orientation almostalways solves that readability problem for me.<br /><br /><pre class="moz-signature" cols="72">-- Nolan Cafferky Software Developer IT Department RBS Interactive <a class="moz-txt-link-abbreviated" href="mailto:nolan.cafferky@rbsinteractive.com">nolan.cafferky@rbsinteractive.com</a></pre>
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