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

[Simo Sorce]

On Sat, 2008-11-01 at 02:55 +0100, Martin Nagy wrote:
> Dmitri Pal wrote:
> > Simo Sorce wrote:

> > >> It is not required but recommended to group and position functions
> > >> in the module so that it is easier to understand.
> > >>     
> > >
> > > This is very controversial, for me easier to understand is when the
> > > innermost function is first in the file and the outermost is last,
> > > but for some that seem an unnatural bottom to top ordering.
> > >
> > >   
> > 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.
> > 
> > I guess this is a personal preference so I would suggest leaving it
> > as it was spelled.
> 
> On this subject, I strongly agree with Dmitri. I always found it
> unnatural that main function is on the bottom, it just doesn't make
> sense. I suspect people do this only because they are lazy to use
> forward declarations.

No, I find it easier to understand that dependencies come first, and
forward declarations break this rule for me.
It really is just 2 different ways to look at the code.

>  This is not a good way. Simo please reconsider,
> I'm sure you will agree with me that code is much more read than
> written. By this logic, I think that if we can get a more easily
> readable code by making just a little more effort, it is worth it. And
> I definitely think this is more readable as it follows logic.

It really depends on what logic path you follow :-)

>  You
> basically only need to read the main function to get the idea what the
> program does. You can then just go the way down as you'd read a book to
> get into more details of the implementation.

s/down/up/ there fixed for ya :-P

> Maybe you don't like the prototypes being on the beginning of the file?

I find annoying to have to fill up prototypes, but I won't die if
everybody prefer prototypes on top :)

> You can quickly skip them and read what's after them. Additionally,
> they serve as a nice reference. I actually always used to write the
> prototypes on the top. Also, if by mistake your declaration is
> different than the definition, the compiler will tell you.

Compiler rules.

> > >> *Comments: *
> > >>
> > >> Sections (groups of functions) can be separated with a line of
> > >> comments like this:
> > >>
> > >> /************ Main Interface Functions ****************/
> > >>
> > >> Or this
> > >>
> > >> /********* Static Internal Functions **********/
> > >>     
> > >
> > > Is it really necessary to state this? Static functions are usually
> > > pretty obviously static ... Also I usually prefer to keep static
> > > functions related to a specific public function close to said
> > > function so that there is no need to jump around the file when
> > > checking what a related function does.
> > >
> > >   
> > Yes. I think it will. It will help you with your approach to
> > processing code to read the code I write and vice versa.
> > These things are very helpful when you face the code that is
> > organized in the way it is not intuitive for you to digest.
> 
> I agree with Dmitri, it's easier to find stuff in a file that has
> sections.
> 
> Also (sorry about the wrapping made by my email client):
> 
> /*
>  * VERY important single-line comments look like this.
>  */
> 
> /* Most single-line comments look like this. */
> 
> /*
>  * Multi-line comments look like this. Make them real
> sentences. Fill
>  * them so they look like real paragraphs.
>  */
> 
> So ideally, no
> /* Multiline comments
>    that look like this */

+1

> I always disliked any ascii art in my files, I always found them
> distracting. Couldn't we just separate the sections with the "VERY
> important" single-line comments? If one wants to really distinguish the
> comment from the others, he might add some empty lines around.

+1

> > >> Each function must have a comment describing what this function
> > >> does and returns.
> > >> There is no specific format for this but it is a good practice to 
> > >> describe what parameters mean and what return codes function
> > >> returns under what conditions.
> > >>
> > >> Inside the function comments should help reader to digest the
> > >> logic of the function. They should answer the main question “why
> > >> is this line of code here and what it tries to accomplish?”. Not
> > >> everything should be commented but practice shows that comments
> > >> giving the hint about what the developer what trying to accomplish
> > >> with this or that construct usually turn out to be pretty helpful.
> > >>     
> > >
> > > This is fine as long as we don't have things like:
> > >
> > > int i = x +1; /* add 1 to x and assign it to i */
> > >
> > > Let's avoid useless comments :-)
> > >
> > >   
> > 
> > Agree.
> 
> +1, the general logic should be:
> Comment on *what* your code does, not *how* it does it. If you really
> must write how it works, you should probably rewrite it.

It really depends on the code, sometimes something may seem wrong but it
is right because it is clever, in that case you might also comment on
why you did something that way.
 
> 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

> > > Function definitions:
> > >
> > > Always define a function on one line with the opening parenthesis
> > > on the next line.
> > > Example:
> > > struct foobar *foo_bar_fn(int foo, int bar)
> > > {
> 
> 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.

Syntax highlighting makes it easy.

>  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. IMO it also makes it easier
> to spot the header of a function. One disadvantage I can think of is
> the case when you need to know the return type in the debugger. But
> this IMO doesn't happen often and so I think it is not that important.
> 
> Are there any arguments against?

I get sick when I see the type and the function on different lines :-)

> +1, but we should use space after standard C keywords, so:
> OK:
> if (expr)
> while (expr)
> 
> NOT OK:
> if(expr)
> while(expr)

+1

> etc.
> 
> > > Avoid complex variable initializations (like calling functions) when
> > > declaring variables like:
> > >
> > > 	int foobar = get_foobar(baz);
> > >
> > > but split it in:
> > > 	int foobar;
> > >
> > > 	foobar = get_foobar(baz);
> > >
> > >   
> > +1
> 
> +1, and don't initialize static or global variables to 0 or NULL.

right

> 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 on all except '\' position :)


> 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

> Try to align members of union and struct definitions, but not at all
> costs:
> 
> struct some_data {
>     int                size;      /* Allocated bytes. */
>     char               *buffer;   /* Buffer containing the names. */
>     struct some_data   *next;     /* Next member. */
>     struct some_name_that_is_very_long  var;  /* Won't fit. */
> };

I find alignment useless and generally harder too read.
When you have more than a handful of variables and the names or values
are far apart I generally find it more difficult to immediately grasp
which refers to what. It makes my eyes keep going left-right many times
to make sure I got the alignment right, waste of time.

It also make you unnecessarily reformat code when you later on add a
variable that moves the alignment further on the right of a few
characters. I am sort of ok for the comment block though, mostly because
it keeps it out of my way when reading a structure I know.

> Creating lists and queues was already done a lot of times. When
> possible, use some common functions for manipulating these to avoid
> mistakes.

+1

> Don't use binary not (!) in tests, unless you test for a boolean:
> good: if (*str = '\0')
> bad:  if (!*str)

I am bad, I tend to use both forms, but I use if (!foo), generally only
to check memory allocations. I am flexible and we can certainly change
to use if (NULL == foo)

> Switches:
> 
> switch (var) {
> case 0:
>     break;
> case 1:
>     printf("meh.\n");
>     /* FALLTHROUGH */
> case 2:
>     printf("2\n");
>     break;
> default:
>     /* Always have default */
>     break;
> }

+1 on switch statement alignment

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York