Hi, SerG.
Thanks for the extra review.
On Tue, Feb 26, 2008 at 02:16:23PM +0100, Sergei Golubchik wrote:
> 1. Why did you need a init_default_directories() variable where you
> store the actual function to be called ? Why not to do it like we do
> elsewhere e.g.
>
> static void init_default_directories()
> {
> #ifdef __NETWARE__
> ...
> #elif defined(__EMX__) || defined(OS2)
> ...
Originally I had coded it like this, and TheK found it difficult to see,
for a particular OS, what the actual behavior is. So I separated it out
into per-OS functions as you see it now, which more in the style of OO
virtual functions.
Here's the preliminary patch which Kristofer objected to:
http://lists.mysql.com/commits/35732
It was intended as a readability improvement. Perhaps it backfired?
> 2. Why ADD_DIRECTORY is defined differently for debug and non-debug
> builds ? Remember, the fewer ifdefs we have in the code, the better.
The sole reason for this was to avoid a warning for the unused return
value variable rc, which is checked with DBUG_ASSERT() but cast to void
in the non-debug version.
I can't put #ifdef inside a macro #define, so this was my solution.
> FIxing 1 and 2, you will be able to remove your utility macros, like
> ADD_DIRECTORY_INTERNAL() and ADD_COMMON_DIRECTORIES() - they aren't
> really necessary, only add a one more redirection step for a reviewer
> (or whoever is reading the code).
>
> The above also made this your patch more difficult to understand than it
> should be.
"Any problem in computer science can be solved by adding another layer of
indirection." Except for the readability problem. ;-)
Timothy
--
-- Timothy Smith Production Engineer; Dolores, Colorado, USA
-- MySQL, www.mysql.com The best DATABASE COMPANY in the GALAXY!
Attachment: [application/pgp-signature]
