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
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
