[Dovecot] coding techniques
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:
There's really no good place to put this little common code, so it's easier to just duplicate it..
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.
participants (2)
-
David Lee
-
Timo Sirainen