[asterisk-dev] [asterisk-commits] rizzo: trunk r89379 - /trunk/include/asterisk/network.h

[Russell Bryant]

Luigi Rizzo wrote:
> On Sun, Nov 18, 2007 at 01:57:29PM -0500, Simon Perreault wrote:
>> On Saturday 17 November 2007 09:34:38 Luigi Rizzo wrote:
>>> The reason for aggregating is
>>> avoid repreating the #ifdef THIS #ifdef THAT all over the place.
>> I feel that's not a good enough reason. The better solution is to put the 
>> #ifdef magic in
>>
>> asterisk/compat/socket.h
>> asterisk/compat/stdint.h
>> asterisk/compat/...
>>
>> , prepend asterisk/compat to the include path, and in the .c's use
>>
>> #include <socket.h>
>> #include <stdint.h>
> 
> that's obfuscation, not good engineering practice, because you make
> the programmer believe that you use the standard stuff and instead
> you don't.

I definitely agree with Luigi on this one.  I think it should be very obvious
that we have done some special handling.  I really like our existing convention
of ...

/* Unmodified system headers */
#include <networkstuff.h>
#include <iostuff.h>

/* Special Asterisk Headers */
#include "asterisk/networkstuff.h"
#include "asterisk/iostuff.h"

It makes it very clear whether we're including a system header or an Asterisk
one that contains special logic and definitions.

> The second reason for aggregating headers is lowering the bar
> for programmers to write code. If we rely on people to know exactly
> which subset of 25 system headers (and in which order) you need on
> the reference linux version for which we created the asterisk/compat/*
> files, we surely end up with lots of inconsistencies and unnecessary
> includes - just browse at the code as it is now and you'll see.
> 
> If you clearly explain some simple instructions such as:
> 
>  - first and foremost, when you include a header don't cut&paste and
>    write a short comment on why you need it (unless it's obvious
>    e.g. compat.h, or you are building a module ...)
>  - all source files need "asterisk/compat.h" which covers the basic
>    C stuff and platform and compiler issues;
>  - if you do something related to system I/O, include "asterisk/fileio.h"
>    (this should cover open/fopen/fcntl/ioctl ...)
>  - if you do something network related include "asterisk/network.h"
>  - if you build a module include "asterisk/module.h"
>  - if you build a channel driver include 'asterisk/channel.h"
>  - if you need the functions supplied by module foobar,
>    include "asterisk/foobar.h"
>  ... (there isn't a lot more, really)
> 
> and the list is reasonably short, then it is easier to write the
> code and for other people to check.
>

This would probably be good information to add to doc/CODING-GUIDELINES so that
new developers are aware of and follow header file handling conventions.

-- 
Russell Bryant
Senior Software Engineer
Open Source Team Lead
Digium, Inc.