Whilst debugging a problem in dovecot LDA over the last few days, I came
across two different issues in coding techniques.
(Note: Please don't take this negatively; my intent is both positive and
constructive!)
The front page of the website www.dovecot.org says:
"it uses several coding techniques to avoid most of the common
pitfalls"
So I hope it is OK to follow the spirit of that with some suggestions.
The two issues I encountered both seem to be fairly easy holes to fall
into, and yet equally easy to avoid in the first place by using
established C practices.
Firstly, many 'if' statements having a single statement (whether in then
or else part) are currently written in a short-cut fashion:
if (condition)
statement 1;
else
statement 2;
This, though technically legal, is potentially lethal when inserting (say)
debugging statements. Without diligence it is all to easy to produce:
if (condition)
debug 1a;
statement 1;
debug 1b;
else
debug 2a;
statement 2;
debug 2b;
A simple edit that looks, layout-wise, as if it would do the right thing
(and do in Python!); but actually radically changes the execution flow.
(This is left as an exercise to the reader... to demonstrate the point.)
Could I suggest that the relevant brackets be always used as a matter of
course? Something like:
if (condition) {
statement 1;
} else {
statement 2;
}
Then the insertion of debug statements doesn't inflict surprise.
(No, I didn't fall into this hole. But it is a trap waiting to happen.)
Secondly, code duplication. To give one example:
'get_var_expand_table()'
appears in both 'deliver/deliver.c' and 'master/mail-process.c'.
(There
is also a similar looking function in 'auth/auth-request.c'.) Perhaps
consideration should be given to merging such instances. For instance, my
woes with "%i"-expansion being treated differently in mail-reading and
mail-delivery would likely never have arisen if common code had been used.
I hope that helps (constructively!).
Best wishes.
--
: David Lee I.T. Service :
: Senior Systems Programmer Computer Centre :
: Durham University :
: http://www.dur.ac.uk/t.d.lee/ South Road :
: Durham DH1 3LE :
: Phone: +44 191 334 2752 U.K. :
On Tue, 2006-09-05 at 17:20 +0100, David Lee wrote:> Firstly, many 'if' statements having a single statement (whether in then > or else part) are currently written in a short-cut fashion: > > if (condition) > statement 1; > else > statement 2; > > This, though technically legal, is potentially lethal when inserting (say) > debugging statements. Without diligence it is all to easy to produce: > > if (condition) > debug 1a; > statement 1; > debug 1b; > else > debug 2a; > statement 2; > debug 2b;If the editor supports automatic indentation properly (like at least mine does), it's practically impossible to do that accidentally.> Could I suggest that the relevant brackets be always used as a matter of > course? Something like: > > if (condition) { > statement 1; > } else { > statement 2; > }Sorry, but I really hate looking at code which looks like that.. :)> Secondly, code duplication. To give one example: 'get_var_expand_table()' > appears in both 'deliver/deliver.c' and 'master/mail-process.c'. (There > is also a similar looking function in 'auth/auth-request.c'.) Perhaps > consideration should be given to merging such instances. For instance, my > woes with "%i"-expansion being treated differently in mail-reading and > mail-delivery would likely never have arisen if common code had been used.I try to avoid duplication as much as possible. In this case there are two reasons why duplication is done: 1) There's really no good place to put this little common code, so it's easier to just duplicate it.. 2) Deliver is still a pretty ugly hack. Hopefully for Dovecot v2.0 I've rewritten it to work differently so that it won't handle the variable expanding itself at all. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 189 bytes Desc: This is a digitally signed message part URL: <http://dovecot.org/pipermail/dovecot/attachments/20061009/1fa8d016/attachment.bin>