Re: [Freeipa-devel] C coding style guide update
On (27/07/15 15:54), Michal Židek wrote:
On 07/26/2015 10:09 PM, Jakub Hrozek wrote:
- 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.
They were already used 2.5 year ago (line 3903)
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3893) void nss_update_initgr_memcache(struct nss_ctx *nctx,
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3894) const char *name, const
char *domain,
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3895) int gnum, uint32_t *groups)
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3896) {
1f15b746 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-19
08:48:06 -0500 3897) TALLOC_CTX *tmp_ctx = NULL;
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3898) struct sss_domain_info *dom;
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3899) struct ldb_result *res;
0c517cb7 src/responder/nss/nsssrv_cmd.c(Jakub Hrozek 2013-01-15
07:05:56 +0100 3900) struct sized_string delete_name;
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3901) bool changed = false;
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3902) uint32_t id;
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3903) uint32_t gids[gnum];
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3904) int ret;
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3905) int i, j;
408914f6 src/responder/nss/nsssrv_cmd.c(Simo Sorce 2012-12-05
17:40:44 + 3906)
LS
--
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
Re: [Freeipa-devel] C coding style guide update
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
Re: [Freeipa-devel] C coding style guide update
On Mon, Jul 27, 2015 at 03:54:22PM +0200, Michal Židek wrote: - 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. This is only personal preference, no technical reason :-) I mostly don't like how they look -- for some reason they are much easier for me to skip visually, even with syntax highlighting. -- 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
Re: [Freeipa-devel] C coding style guide update
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).
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.
- Line-comments (//, aka C++ comments) should be still avoided,
though
- Variable Length arrays are very helpful, but explicitly mention
they should be used with caution, especially if array size might
come from the user
- 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.
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.
It's not portable, but seriously...are there any compilers except gcc
and clang that are used at all these days??
Location of the C coding style guide was discussed on FreeIPA dev call.
Conclusion was the same as Jakub's opinion: no fork, link from the
other, it doesn't matter if it is in SSSD or FreeIPA wiki.
--
Petr Vobornik
--
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
Re: [Freeipa-devel] C coding style guide update
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).
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.
- Line-comments (//, aka C++ comments) should be still avoided,
though
- Variable Length arrays are very helpful, but explicitly mention
they should be used with caution, especially if array size might
come from the user
- 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.
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.
It's not portable, but seriously...are there any compilers except gcc
and clang that are used at all these days??
--
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
