[Freeipa-devel] C coding style guide update

[Michal Židek]

On 07/26/2015 10:09 PM, Jakub Hrozek wrote:
> On Thu, Jul 23, 2015 at 06:21:25PM +0200, Michal Židek wrote:
>> Hi,
>>
>> in SSSD we use the freeipa coding guidelines which are located here:
>> http://www.freeipa.org/page/Coding_Style
>>
>> However this coding style guide is already dated and there are
>> some rules we follow in SSSD which are not mentioned in the guide
>> and also there are some C language features that we would like to
>> start using in certain way but their usage should be covered in the
>> coding style guide. So, update is needed (at least for SSSD).
>>
>> I would like to start discussion about what to add to the coding
>> guide (and maybe what to remove), but before that, I would like
>> propose to move the coding style guide to SSSD wiki and just add link
>> to it to FreeIPA wiki. The reason is that unlike FreeIPA, most of the
>> SSSD code is written in C and SSSD team will more likely update and
>> modify the guide according to new practices used in upstream
>> development, where FreeIPA is mostly Python project and C coding
>> style probably does not need revision as often. So SSSD wiki
>> seems like more appropriate place.
>>
>> Another possibility would be to fork the FreeIPA style and
>> maintain SSSD coding style guide separately. But I think linking
>> the two is better option, because the two projects are closely
>> related and it makes sense to share the coding style guidelines.
>>
>> So, my first question is, Is someone against moving the C coding
>> style guide to SSSD wiki and adding link to it on FreeIPA wiki?
>
> I don't really mind where the coding style is located as long as it's
> on one place (no forks please) and the existing link points to a new
> version (if any).

Ok. I will start crafting the new SSSD wiki after we come to some
conclusion in this thread.

>
> As per updating the coding standards, I would like to propose to:
>      - explicitly say that C99 is fine to use. It's 2015 and any compiler
>        that doesn't support C99 at this point is probably dead and should
>        be avoided (Hello, MSVC!). We use stdbool.h and variadic macros
>        already anyway.

+1

>      - Line-comments (//, aka C++ comments) should be still avoided,
>        though

I really do not know what people have against line comments, but
this is not the first time I see someone resisting them, so I
guess there is some hidden evil in this way of commenting the code.
But I am OK if they stay forbidden.

>      - Variable Length arrays are very helpful, but explicitly mention
>        they should be used with caution, especially if array size might
>        come from the user

+1
We overuse talloc for very small allocations that can be done
automatically on stack.

>      - Also, I would warn about interleaved variable declarations. I
>        think it's fine to declare some helper variable inside a for loop
>        for example, but generally it might be better to refactor the
>        function if we find out there's so many variables that the code
>        author ends up declaring them inside blocks.

It is good practice to declare variables at the begging of the
block that covers all blocks where the variable is used.
And it is one of the things I would like to put in the
coding style. I am not sure about loops however. it could lead
us to hard to debug bugs if someone forgets to put static keyword
in variable declaration.

>
> Personally, I would even go as far as to allow the __cleanup__
> attribute. I really like how the systemd codebase uses it to define
> helper "destructors" like:
>      int closep(int fd)
>      {
>          if (fd >= 0) {
>              close(fd);
>          }
>      }
>
>      #define _cleanup_close_ _cleanup_(closep)
>
> Then safely declare a file descriptor as:
>      _cleanup_close_ int fdf = -1;
> ..and stop worrying about closing the fd in all branches.

Looks like a good thing to me as well for the cases when
we *always* want to destroy the resource before leaving
the function. For the rest of the cases we would still
have to use goto labels.

>
> It's not portable, but seriously...are there any compilers except gcc
> and clang that are used at all these days??

GCC and Clang are the most widely used compilers on platforms we
care about. We do not need to make SSSD compile on anything else.

We could also add few tips and 'rules of thumb' to the coding style
as well. For example isolating the untrusted value on the left
side when doing comparisons in ifs ( see ticket
https://fedorahosted.org/sssd/ticket/1697 ).

Michal

-- 
Senior Principal Intern