Coding style
Stephen Gallagher
sgallagh at redhat.com
Fri Nov 8 13:43:36 UTC 2013
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 11/08/2013 03:37 AM, Marius Vollmer wrote:
> Stephen Gallagher <sgallagh at 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-next-project/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.15 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlJ86ogACgkQeiVVYja6o6O7CwCfcW2VwuAOplettoIOqIAuhAQ1
KR8An0s7OnZd4a2OBauKFAJ16xHT/ATU
=NtH9
-----END PGP SIGNATURE-----
-------------- next part --------------
#define MEMCHECK(var) \
do {
if (!var) { \
ret = ENOMEM; \
goto done; \
}
} while 0;
struct userdata {
gchar *givenname;
gchar *surname;
gchar *fullname;
gchar *username;
void *pvt;
};
/* Opaque function */
gchar *construct_username(TALLOC_CTX *mem_ctx,
gchar *givenname,
gchar *surname);
errno_t func(TALLOC_CTX *mem_ctx,
gchar *first, gchar *last,
struct userdata **_data)
{
errno_t ret;
gchar *res = NULL;
struct userdata *data = NULL;
TALLOC_CTX *tmp_ctx = talloc_new(NULL);
MEMCHECK(tmp_ctx);
data = talloc_zero(tmp_ctx, struct userdata);
MEMCHECK(data);
data->surname = talloc_strdup(data, last);
MEMCHECK(last_name);
data->givenname = talloc_strdup(tmp_ctx, first);
MEMCHECK(first_name);
data->fullname = talloc_asprintf(data, "%s %s",
data->givenname,
data->surname);
MEMCHECK(first_name);
data->username = construct_username(data,
data->givenname,
data->surname);
MEMCHECK(data->username);
/* Everything succeeded */
ret = 0;
/* This operation cannot fail */
*_data = talloc_steal(mem_ctx, data);
done:
talloc_free(tmp_ctx);
return ret;
}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: example.c.sig
Type: application/pgp-signature
Size: 72 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/cockpit-devel/attachments/20131108/5efc8731/attachment.sig>
More information about the cockpit-devel
mailing list