Hi,
let's nail down our coding style guidelines. I have this so far, discuss!
Cockpit Coding Guidelines =========================
* General
- No trailing whitespace, no tab characters - Maximum line length is 120 characters.
* C
- Gtk+ coding standards. - C99 is allowed and the default. - __attribute((cleanup)) is allowed, via libgsystem.
* JavaScript
- Crockford
* Python
- PEP 8
* CSS / HTML
- XXX dashes vs underscores in ids. - XXX namespacing of ids.
* Emacs
(setq indent-tabs-mode nil) (add-hook 'before-save-hook 'delete-trailing-whitespace) (setq whitespace-style '(face trailing lines-tail empty)) (setq whitespace-line-column 120) (global-whitespace-mode)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 11/06/2013 07:24 AM, Marius Vollmer wrote:
Hi,
let's nail down our coding style guidelines. I have this so far, discuss!
Cockpit Coding Guidelines =========================
- General
- No trailing whitespace, no tab characters - Maximum line length
is 120 characters.
Could we make this 80 characters? I for one like being able to display two files side-by-side in vim or eclipse, and 120 characters makes this difficult.
- C
- Gtk+ coding standards. - C99 is allowed and the default. -
__attribute((cleanup)) is allowed, via libgsystem.
I realize I'm likely to get outvoted on this, but I dislike any mechanism that encourages lazy memory management. It's twelve o'clock; do you know where your pointers are?
- JavaScript
- Crockford
- Python
- PEP 8
+1[000]
- CSS / HTML
- XXX dashes vs underscores in ids.
I vote underscores
- XXX namespacing of ids.
- Emacs
(setq indent-tabs-mode nil) (add-hook 'before-save-hook 'delete-trailing-whitespace) (setq whitespace-style '(face trailing lines-tail empty)) (setq whitespace-line-column 120) (global-whitespace-mode)
* Vim
autocmd ColorScheme * highlight ExtraWhitespace ctermbg=red guibg=red set hlsearch highlight ExtraWhitespace ctermbg=red guibg=red match ExtraWhitespace /\t|\r|\S.*\s+$|^\s\s+$|%>79v.+/ set si set ts=4 set sw=4 set sta set sts=4 set et set tabstop=4 set shiftwidth=4 set expandtab set guifont=Monospace\ 12 set tw=79
Stephen Gallagher sgallagh@redhat.com writes:
- No trailing whitespace, no tab characters - Maximum line length is 120 characters.
Could we make this 80 characters?
Hmm, no. :-)
At least not now. The code mostly respects the 120 limit right now, and this used to be documented in the old HACKING file. So changing this would cause quite some work.
- Gtk+ coding standards.
- C99 is allowed and the default.
- __attribute((cleanup)) is allowed, via libgsystem.
I realize I'm likely to get outvoted on this, but I dislike any mechanism that encourages lazy memory management. It's twelve o'clock; do you know where your pointers are?
The jury is still out on this, I'd say. I like uncluttered code that shows the logic, and I think "gs_free" and friends can help a lot. I think a function like this is quite readable and maintainable:
gchar * func () { gs_free gchar *last_name = dup_last_name (); if (last_name == NULL) return NULL;
gs_free gchar *first_name = dup_first_name (last_name); if (first_name == NULL) return NULL;
return g_strdup_printf ("%s %s", first_name, last_name); }
This looks much worse to me:
gchar * func () { gchar *last_name = NULL; gchar *first_name = NULL; gchar *result;
last_name = dup_last_name (); if (last_name == NULL) return NULL;
first_name = dup_first_name (last_name); if (first_name == NULL) { g_free (last_name); return NULL; }
result = g_strdup_printf ("%s %s", first_name, last_name); g_free (first_name); g_free (last_name);
return result; }
... mostly because one needs to duplicate the cleanup code a lot. "goto out"-style is better:
gchar * func () { gchar *last_name = NULL; gchar *first_name = NULL; gchar *result = NULL;
last_name = dup_last_name (); if (last_name == NULL) goto out;
first_name = dup_first_name (last_name); if (first_name == NULL) goto out;
result = g_strdup_printf ("%s %s", first_name, last_name);
out: g_free (first_name); g_free (last_name); return result; }
But using "gs_free" is still better IMO because the return value is much more explicit. In a sense, with "gs_free" the compiler is doing the transformation to "goto out"-style for us, which ought to be a good thing.
On Fri, 2013-11-08 at 10:37 +0200, Marius Vollmer wrote:
But using "gs_free" is still better IMO because the return value is much more explicit. In a sense, with "gs_free" the compiler is doing the transformation to "goto out"-style for us, which ought to be a good thing.
Yeah, I'm pretty strongly of the opinion that it's a good thing. It's also heavily used by systemd now, among other projects.
It's not like garbage collection or something, the free() calls are still predictable and reliable.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 11/08/2013 03:37 AM, Marius Vollmer wrote:
Stephen Gallagher sgallagh@redhat.com writes:
- No trailing whitespace, no tab characters - Maximum line
length is 120 characters.
Could we make this 80 characters?
Hmm, no. :-)
At least not now. The code mostly respects the 120 limit right now, and this used to be documented in the old HACKING file. So changing this would cause quite some work.
- Gtk+ coding standards. - C99 is allowed and the default. -
__attribute((cleanup)) is allowed, via libgsystem.
I realize I'm likely to get outvoted on this, but I dislike any mechanism that encourages lazy memory management. It's twelve o'clock; do you know where your pointers are?
The jury is still out on this, I'd say. I like uncluttered code that shows the logic, and I think "gs_free" and friends can help a lot. I think a function like this is quite readable and maintainable:
gchar * func () { gs_free gchar *last_name = dup_last_name (); if (last_name == NULL) return NULL;
gs_free gchar *first_name = dup_first_name (last_name); if (first_name == NULL) return NULL;
return g_strdup_printf ("%s %s", first_name, last_name); }
This looks much worse to me:
gchar * func () { gchar *last_name = NULL; gchar *first_name = NULL; gchar *result;
last_name = dup_last_name (); if (last_name == NULL) return NULL;
first_name = dup_first_name (last_name); if (first_name == NULL) { g_free (last_name); return NULL; }
result = g_strdup_printf ("%s %s", first_name, last_name); g_free (first_name); g_free (last_name);
return result; }
... mostly because one needs to duplicate the cleanup code a lot. "goto out"-style is better:
gchar * func () { gchar *last_name = NULL; gchar *first_name = NULL; gchar *result = NULL;
last_name = dup_last_name (); if (last_name == NULL) goto out;
first_name = dup_first_name (last_name); if (first_name == NULL) goto out;
result = g_strdup_printf ("%s %s", first_name, last_name);
out: g_free (first_name); g_free (last_name); return result; }
But using "gs_free" is still better IMO because the return value is much more explicit. In a sense, with "gs_free" the compiler is doing the transformation to "goto out"-style for us, which ought to be a good thing.
I'm a big fan of hierarchical memory management, myself[1].
See attached for an example of how this works. The major advantages are that you always know where your memory is and you can destroy an entire object recursively with a single talloc_free().
In the example, all memory in a function is allocated directly or indirectly as a child of the tmp_ctx. If at any point, the function has to exit due to error, it can simple call talloc_free(tmp_ctx) and know that all allocated memory (whether or not it occurred in this function or a subroutine!) is cleaned up.
[1] http://sgallagh.wordpress.com/2010/03/17/why-you-should-use-talloc-for-your-...
On Fri, 2013-11-08 at 08:43 -0500, Stephen Gallagher wrote:
I'm a big fan of hierarchical memory management, myself[1].
talloc is cool and is developed my competent people, and makes sense. But the Cockpit codebase is using GLib, and when you start talking about blending the two, it gets ugly - you need to worry about two allocators, which call to free which pointer, etc.
In the end even with GLib we often end up with a hierarchy because GObjects have "dispose" handlers which unreference the objects they manage, and with any refcounting system you obviously can't create circular references.
cockpit-devel@lists.fedorahosted.org
-
Colin Walters -
Marius Vollmer -
Stephen Gallagher