Re: commit into mysql-6.0-wtf:mysql-6.0-wtf branch (vvaintroub:2653) Bug#24509, WL#3049

[Davi Arnaut]

Joerg Bruehe wrote:
> Hi Davi !
> 
> 
> Davi Arnaut wrote:
>> Hi Vlad,
>>
>> Vladislav Vaintroub wrote:
>>> #At bzr+ssh://bk-internal.mysql.com/bzrroot/server/mysql-6.0-wtf/
>>>
>>>  [[...]]
>> Wouldn't this be a good opportunity to start to untangle the ifdefery
>> mess in mysys? We could start splitting the implementations into
>> different directories, for example:
>>
>> mysys/win32/file_io.c
>> mysys/unix/file_io.c
>> mysys/netware/file_io.c
>>
>> And each one would implement it's own specified version of my_foo.
> 
> I have a very strong feeling against such a split into separate files 
> for platform-specific implementations.
> 
> 
> Sure, it would get rid of several "#if"/"#ifdef"/... and so make reading 
> a bit easier.
> 
> 
> But it would also cause several source modules (files) which have to 
> implement the same interface (for the caller), so any change to that 
> interface would need to be done in several files in sync.

Sure.

> As long as these various implementations are in the same file, it is 
> easy to search for the relevant identifier(s) using your editor, and 
> find all occurrences.

It's also easy to use grep.

> Hopefully, it occurs only once, and any differentiation occurs in its 
> body - this guarantees an interface change is done in the same way for 
> all implementations.

This is not always the case as the implementation can significantly
differ, for example, for condition variables or the thread abstraction.
I suggest this for big ones that covers dozens of lines.

Also if a interface changes happens, the prototype is modified
(include/file_io.h) and all implementations of the said need to be
modified (three or four different files) or compilation will fail.

Each (portable) interface should also be tested in all supported
platforms, where different behavior would be spotted.

> The moment you have multiple modules (by implementation) for one 
> interface, keeping them in sync gets much more complicated - even more 
> so, because your local testing will typically be on one platform only.

IMHO, editing three of four different files is not so complicated. Also,
our local testing should cover all platforms or at least test all
compile test all platforms.

> If you really feel different implementations should be split into 
> different files, at least keep one common entry (mapper) module for 
> them. (The drawback of this is of course reduced chances for the 
> optimizer, as now the call crosses file boundaries.)

IMHO not necessary, proper review and testing should cover this. If a
reviewer sees a modification of the behavior of one implementation and
does not touch others, a red flag is raised.

> IMHO, a mapper function for the interface, with its body being just some 
> "#if" around calls to different implementations, and these being defined 
> as "static inline" functions (completely within large "#if" blocks) is 
> as readable, and it allows for optimization.
> 
> I have seen too many cases where developers changed one occurrence only 
> (which they had tested) and forgot others (which "bk -r grep ..." would 
> have immediately shown to them).

Well, sh*t happens no matter what. Another common problems with the
ifdef approach: developers declaring variables in a different ifdef that
they are used or using variables not declared in their context, headers
interfering, unbalanced ifdefs, etc.

Either one has particular problems, but in the end it makes more sense
to have code that is easy to read and maintain, this eventually leads to
less bugs and more contributions.

Regards,

-- 
Davi Arnaut, Software Engineer
MySQL Server Runtime Team
Database Group, Sun Microsystems