[Olsr-dev] questions regarding v2 coding style / lang

[Henning Rogge]

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), 
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.

> > code only because it saves a few lines if the longer (and more readable)
> > code would result in a similar binary.
>
> Yes but "readability" is rather subjective. Personally I find the "0 ==
> xx" completely unreadable (And yes, I know why people do that. And that
> is with gcc with it's warnings since years not really necessary
> anymore).
Yes, that's true... it's just a "rule of thumb"... if it's becoming too 
complex, split it into multiple lines.

> > No scripts to create code. Generated code is a nightmare to read and to
> > debug.
>
> *eg* Only if your generator is crap;-) Let him generate readable code.
> And no one really debugs generated code. You debug and fix the
> generator[2].
> At most one reads the generated code to improve the generator.
>
> If you have a generator, it must check the input completely (and reject
> it if it has bugs[4]).
>
> And generating code something implies that you have input data which
> will change overtime quite frequently (and by people which are not
> expected to fix the source) and it is less work and risk to have that
> additional tool. There is no need to generate code for a bunch of never
> changing data.
>
> BTW: If your generator generates lots of code, you very probably doing
> something wrong anyways. In my experience, the generators evolve into
> producing tables which drive normal functions.
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.

> 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.

> 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)

> 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.

> Define variables in the most inner block.
> Rationale: No one wants to scroll up and look for the definition.
Good idea.

> Use meaningful function and variable names.
> Rationale: It makes understanding the code easier and avoids explicit
> comments.

> 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.

> Rationale: A type cast completely removes type checks of the compiler.
> And we won't educate people to use casts[0] but the other way around.
> Exception: Casts can be used to type-safety. Use it then.

> 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 
be mapped on different things depending on the compiler/machine.

> 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 !

> Make all .c and .h files self-confident.
> Rationale: You do not want to look into foo.h after you include it in
> bar.c and add 3 more #include lines just because foo.h needs them.
>
> Minimize the number #include's (without violating the previous).
> Rationale: Apart from the neglectable performance win, one can see at
> one glance what stuff from which other .c files is used.
>
> I see a .c file as the lowest level of modularization. So have 1 .h file
> for every .c file containing the relevant extern's and #defines.
> And #include that in it's .c file as the very first #include.
> Rationale:
> - Think of "OO design" (and yes, an object-oriented programming
>   language is neither necessary nor sufficient for OO programming. It
>   makes it easier and helps but that's all).
> - That guarantees the above "self-confidence".
> - So the compiler actually checks if the function declaration in the .h
>   file actually matches the function definition (and yes, even olsrd had
>   such differences which crept into it over time).
>
> Always use the { ... } for if()/else, while(), for(), do { }
> while(), ... No oneliner if(),while(), etc. - have the body on separate
> lines.
> Rationale: You have then one pattern of these (and not two) and reduce
> the risk of conflicts on merging.
>
> ad formatting: Use Linux kernel coding style[3] except
> - the above "use { ... } everywhere"
> - indent 4 (and not 8) positions.
> - indent with spaces (and not tabs)
>   Rationale: It is far easier to get it right. And there is only one
>   true width of one tab - and that is 8.
> a draft list of parameters for indent is
> ---- snip  ----
> --k-and-r-style --indent-level4 --tab-size8
> --swallow-optional-blank-lines --line-length132
> --space-special-semicolon --no-space-after-casts --braces-on-if-line
> --braces-on-struct-decl-line --no-tabs --cuddle-else --cuddle-do-while
> ---- snip  ----
>
> 	Bernd

> [0]: IMHO *every* single type cast should have an comment explaining why
>      it is needed. And *root cause* has to be explained, not a symptom
>      like "gcc throws a warning and that got rid of it".
>      That holds especially for beginner courses in C.
I think "const" is one of the reasons for many casts. ;)

Henning


*************************************************
Diplom Informatiker Henning Rogge
Forschungsgesellschaft für
Angewandte Naturwissenschaften e. V. (FGAN) 
Neuenahrer Str. 20, 53343 Wachtberg, Germany
Tel.: 0049 (0)228 9435-961
Fax: 0049 (0)228 9435-685
E-Mail: (spam-protected)
Web: www.fgan.de
************************************************
Sitz der Gesellschaft: Bonn
Registergericht: Amtsgericht Bonn VR 2530
Vorstand: Dr. rer. nat. Ralf Dornhaus (Vors.), Prof. Dr. Joachim Ender 
(Stellv.)