
On Mon, 2008-09-08 at 12:41 +0200, Henning Rogge wrote: 
> Am Montag 08 September 2008 11:43:36 schrieb Bernd Petrovitsch:
> > > No code in header files, it does belong into the source files.
> >
> > ... unless it is in inline functions.
> If it's necessary to put inline functions in header files (I don't think so), 

How can the compiler inline a function he doesn't know (because the body
is buried in some other .[co] file)?
At most the linker could in theory do such a job (and use a good part of
the compiler for it).
There is nowadays the -ffunction-sections and -fdata-sections for gcc
(to place each function and global variable in their own section) and
--gc-sections for the linker to throw out all unused data and functions.
But we can't use that since we have function in the core-olsrd (and
which belong there from the design point-of-view. Last time I tried that
one really belongs to one of the tables and the others mainly the plugin
parameter helpers) which are used only by plugins.
One could place them into the plugin it needs, but that would - at least
- move a function fiddling with internals into a plugin and duplicate
the code (and all depending on which plugins you ship and use).

I can see a nice and simple solution of the problem.
Perhaps the plugin abstraction/interaction can be done better.

> then we should not use many inlines and just ask the compiler to optimize the 
> code.
> 
> > And contrary to common belief: It is a myth (and/or cargo cult), that
> > simply inlining C code makes it faster.
> > Yes, there are situations where this is true (like one line accessors).
> > But as soon as you have one if()[1] or while() (or - God beware - a
> > system call), the cost of an additional function call is very probably
> > neglectable compared to the rest of the function. And the #define/inline
> > normally blows up the generated code bringing more pressure on the L1/L2
> > caches and the memory accesses (and this is even more the case the
> > larger/faster the CPU is).
> > That's actually the reason for the -finline-limit parameter: To catch
> > cases where an inline function suddenly explodes (for whatever reason).
> So maybe we should not inline ourself but just let the compiler decide.

gcc was notoriously bad for this - read: inline'd too much. And for
others I have even less estimations.
BTW according to the C standard the "inline" keyword was never nothing
more than a hint to the compiler (and he is free to ignore it for
whatever reason, e.g. "not implemented").
I use (and see) inline functions only as "type-checking macros" (in the
nearest sense). So anything more than some one- or two-liner (see the
ones in src/ipcalc.h as the somewhat largest that makes sense IMHO.
Granted, a few of them are debatable) shouldn't be inlined in the first
place. Of course if someone shows good reasons, I have no problems with
exceptions to rules (including a comment here so that it is not
forgotten).
One use for macros is to transparently hide __FILE__ and __LINE__
parameters.

[...] 
> I think our config file parser is one of the BAD examples... it's always a 
> nightmare to understand what this thing is doing and how to add/change an 
> option.

It's a typical lex/yacc parser. And yes, they do not produce the most
beautiful code in the world. But people (at least me) are old enough to
know lex/yacc good enough to use it (and my sed scripts are there to
kill the warnings. We can also disable the warnings for the relevant
files but adding the `sed` stuff was simpler as it was in exactly two
places).

For starters on lex/yacc: find another similar option and copy-edit it.

The only chance that I see: Throw it out and replace it with something
better.

> > If someone is interested in mine (additional to the above;-):
> >
> > The most important one: Do not copy "logic" (read: almost always code).
> > Abstract it. Or will have a maintenance nightmare sooner or later.
> Yes, abstraction is a good idea.
One can over-abstract too - especially on the higher levels and trashing
the performance through enormous use of malloc()/free() onm large
networks.

> > Declare file local functions and data "static". Otherwise give a
> > declaration in it's header file.
> (this is similar to the use of public/private in C++... don't put internal 
> functions into the visible interface)
Yup.

> > Use "const" if a variable shouldn't/isn't changed.
> > Rationale: One can easily see it and the compiler checks it too.
> Const is a mostly useless addition in C. It just force you to have two 
> functions for each original one (one const, one normal) or use lot's of 
> casts.
Yes, sometimes (especially with old libs like xdr, rpcgen etc.) that
gets messy (and "free(void *)" is broken defined in the C
standard/POSIX/SUSv3 BTW. That should better be "free(const void *)").

And then you are doing something wrong. The duplicated set of accessors
in src/common/avl.h is IMHO a strong sign that we should get rid of them
completely.
I see no real value in them anyways: for "hiding of internals" to the
rest of the world they are IMHO too low level (and one wouldn't use them
internally anyways).
And what's their practical value?

[...] 
> > Remove all useless casts, e.g. casting the return value of malloc() and
> > friends since void * are automatically convertible anyways.
> Not casting the output of malloc() produce a warning/error, depending on the 
> compiler settings.

Then fix you *C-compiler* and/or it's settings.
If the compiler produces a warning, you are *not* using a C-compiler
(but more probably a C++ compiler).
Sorry, "people with misconfigured and/or br0ken environments" are not a
good reason IMHO.
If someone really wants to support C++ compilers (for whatever reason),
please find a saner solution than to add type casts (and/or disable
warnings).

[...] 
> > Use "typedef" only for function types.
> > Rationale: All other uses obfuscate the source - especially true for
> > struct's and union's (as one forgets that you have a struct or union
> > there. And you won't look up .h fiels all the time).
> > Exceptions: You want some special kind of e.g. "int" to be used with a
> > special set of functions and use a typedef to make the type check
> > trigger wrong uses. "atomic_t" is the prime example in the Linux kernel
> > code.
> I see typedefs usable to hide machine specific variable types... olsr_u8_t can 

I lack to find the requirements, description and details of that for the
appropriate places for the next port;-)
So it is IMHO information hiding - read: obfuscation - for no good
reason (as we don't hide the implementation of some creepy algorithm and
internal data structures).

> be mapped on different things depending on the compiler/machine.

Yes, I know 0.4.10. See how it looked like at that time;-)

And that example is not that good either - <inttypes.h> already does
this for us since ages:
http://www.opengroup.org/onlinepubs/009695399/basedefs/inttypes.h.html

So I'm more the "do `find -name '*.[ch]' -print0 | xargs -0r perl -w -i
-p -e 's/olsr_u8_t/uint8_t/' and remove the typedef" person.
Hmm, I have to try <inttypes.h> on Win32.

The counter example is (people, don't take it personal): olsr_cookie_t
- The "_t" suffix is actually reserved for future libc types. But that's
  more a stylistic problem than something else (as the C compiler will
  complain on a clash).
- It is 16bit - so far so good. But everyone has to remember that. Not
  only now while coding it but also in a year or two.
- I read some code in a year.
  Does this code overflow in that situation or not?
  * And I grep for the typedef to find out the size.
  Why doesn't this works the expected way?
  * Oh, it's not an int
Apart from padding issues etc. - remember the bug with "sizeof(struct)
vs offsetof(field,struct)" bug that Sven-Ola hunted down.
If "uint16_t" is everywhere, at least one can see immediately that
padding may be important.

And please take the above "olsr_cookie_t" to the extreme. Imagine all
sorts of different olsr_whatever_t's (some enums, several different
int's, some float's, some double's..) for all the different
protocol-specific numbers (e.g. all values you can configure in
olsrd.conf).
The problem is not if you are deep into development/debugging of that
module but if you look at it 6 months later .....
Or if you try to review the code and have to learn all sorts of
typedefs. That doesn't save time on the reviewers side and thus doesn't
motivate that much.

JftSoC: IMHO libc/C-standard/POSIX/SUSv3 brings already far too many
subtle different types - size_t (e.g. write(2) accepts that), ssize_t
(e.g. write(2) returns that), ptrdiff_t, ...
Does everyone know without digging into docs if they are signed or
unsigned and how large they are (either absolute or in terms of "int")
for 32bit and 64bit archs?
The obvious "problems" are printf-like format strings. I don't like the
"cast to long" everywhere to make it work on 32bit and 64bit - using the
appropriate macros from the above site are even less readable (and IMHO
the performance degradation - if there is any - on 64bit is nothing
compared to somewhat unreadable source code).

> > Activate all remotely useful of warnings - especially -Werror. And yes,
> > e.g. -Wshadow (and others) let me find bugs and quirks and superfluous
> > stuff in other apps.
> Addition to this:
> Valgrind is your friend... use it !
ACK. And all sorts of other tools.

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services





