[Freeipa-devel] Suggested additions to codding standard page on freeIPA wiki.

[John Dennis]

Martin Nagy wrote:
> I like using typedefs if the convention is to always end them with _t.
> However, this way you sometime need both struct and typedef (self
> reference, as in case of linear lists for example). I also like structs
> alone and this approach is much more clearer, because there's no need
> to mix typedefs and structs. And it's always apparent at first glance
> that we're dealing with a struct as Simo noted, so I'm vouching for
> structs.
>
> Also, let's not forget that hiding a pointer behind a typedef is a
> mortal sin.
>
>   
+1
>> No it is not. I think this allows you to follow your natural flaw of 
>> logic and me mine.
>> I prefer implementing the functions that are main to the module first 
>> and stub out the functions that I need.
>> So naturally I read the code the same way. I do not care about the 
>> details and low level functions that act as helpers.
>> I want to see and concentrate on the core functionality and not dig
>> it into the module to find it scattered somewhere in the middle or in
>> the bottom.
>>
>>     
This ordering makes sense, but I've learned to read code in both styles 
so I don't really care. There really isn't a right or wrong way, both 
have valid reasons behind them so just let it be a personal preference 
of the author with the only caveat being the author is ultimately 
responsible for making the entire file readable and self consistent (the 
later being most important).
> Which reminds me of another thing: If appropriate, always use the
> const modifier for pointers passed to the function. This makes the
> intentions of the function more clearer, plus allows the compiler to
> catch more bugs and make some optimizations.
>
>   
+1
Not really a coding style but rather proper software engineering, but 
yes this should always be done.
> I disagree with the position of the type. There are a lot of types with
> variable length and with both the type and function name on one line,
> it's hard to quickly find the name of the function. In contrast, it is
> much easier to find it when it's on the beginning of a line and the
> type and name are easily distinguishable.
+1 see previous email
> +1, but we should use space after standard C keywords, so:
> OK:
> if (expr)
> while (expr)
>
> NOT OK:
> if(expr)
> while(expr)
>   
+1

> Macros that are unsafe should be in upper-case. This also applies to
> macros that span multiple lines:
>
> #define MY_MACRO(a, b) do {                       \
>              foo((a) + (b));                      \
>              bar(a);                              \
> } while (0)
>
> Notice that arguments should be in parentheses if there's a risk, but
> in case of bar(a) this isn't the case (caution however won't hurt).
> Also notice that a is referenced two times, and hence the macro is
> dangerous. Wrapping the body in do { } while (0) makes it safe to use
> it like this:
>
> if (expr)
>     MY_MACRO(x, y);
>
> Notice the semicolon is used after the invocation, not in the macro
> definition. Also notice the position of \'s.
>
> Otherwise, if a macro is safe (for example a simple wrapping function),
> then the case can be lower-case. Reasoning is that too much upper-case
> looks ugly.
>
>   
+1
> When using #ifdefs, it's nice to add comments in the pairing #endif:
>
> #ifndef _HEADER_H_
> #define _HEADER_H_
>
> /* something here */
>
> #endif /* !_HEADER_H_ */
>
> or:
>
> #ifdef HAVE_PTHREADS
>
> /* some code here */
>
> #else /* !HAVE_PTHREADS */
>
> /* some other code here */
>
> #endif /* HAVE_PTHREADS */
>
>
>   
+1
>
> Creating lists and queues was already done a lot of times. When
> possible, use some common functions for manipulating these to avoid
> mistakes.
>
>   
+1

Actually it would be really nice we could agree on a basic helper 
library for implementing common data structures and operations. FWIW 
I've found glib to be pretty useful and widely available. But I'm sure 
others will have strong feelings about the merits of any such given 
library so let the flames begin :-)
> Don't use binary not (!) in tests, unless you test for a boolean:
> good: if (*str = '\0')
> bad:  if (!*str)
>
>   
I somewhat agree, I'm O.K. with "if (!*str)", it's compact (especially 
in loop constructs) and easy to read. But there are other cases where 
the ! operator is misleading.

I find this coding style counter-intuitive to read:

if (!strcmp(a,b))

I prefer

if (strcmp(a,b) == 0)

-- 
John Dennis <jdennis at redhat.com>