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