Re: GError implementation

[Tim Janik <timj gtk org>]

On 23 Jun 2000, Havoc Pennington wrote:

> 
> Comments?
> 
> /* gerror.h - Error reporting system
>  *
>  *  Copyright 2000 Red Hat, Inc.
>  *
>  * The Gnome Library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Library General Public License as
>  * published by the Free Software Foundation; either version 2 of the
>  * License, or (at your option) any later version.
>  *
>  * The Gnome Library is distributed in the hope that it will be useful,
>  * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>  * Library General Public License for more details.
>  *
>  * You should have received a copy of the GNU Library General Public
>  * License along with the Gnome Library; see the file COPYING.LIB.  If not,
>  * write to the Free Software Foundation, Inc., 59 Temple Place - Suite 330,
>  *   Boston, MA 02111-1307, USA.
>  */
> 
> #ifndef __GERROR_H__
> #define __GERROR_H__
> 
> #ifdef __cplusplus
> extern "C"
> {
> #endif
> 
> typedef struct _GError GError;
> 
> struct _GError
> {
>   GQuark       module;

i already said this to owen, do s/module/domain/ to saty consistent with
the g_log() naming scheme and avoid confusion with GModules.

>   gint         code;
>   const gchar *str;

can we write this out? "string" isn't an awfully long name, other
candidates are "description", or to think of the Gimp's API "blurb".

>   gpointer     data;
> };
> 
> #define g_error_matches(error,mod,num) (error && error->module == (mod) && error->code == (num))

why would a NULL pointer be a valid error? usually you have error codes like
G_ERROR_OK=0 or G_ERROR_NONE=0. also it is a very bad idea to hide multiple
argument in a function alike macro, we use upper case macros for this:
G_ERROR_MATCHES() and even then try to avoid multiple argument evaluations.
for this specific case, i don't see why you couldn't use a normal function,
error code evaluation is rarely time critical, and even then, glib deals with
inline functions quite nicely.

> GError*  g_error_new           (GQuark         module,
>                                 gint           code,
>                                 const gchar   *format,
>                                 ...) G_GNUC_PRINTF (3, 4);
> 
> 
> GError*  g_error_new_with_data (GQuark         module,
>                                 gint           code,
>                                 gpointer       data,
>                                 GDestroyNotify dnotify,
>                                 GDataCopyFunc  copyfunc,
>                                 const gchar   *format,
>                                 ...) G_GNUC_PRINTF (6, 7);

huh? ok there's no GDestroyNotify function in GError, probably an omission,
but what's that with GDataCopyFunc? where will you store that? and btw,
you're pollution the already used GData namespace.
all in all this looks much like duplicating what GValue does (just a bit
more generic).
also a custom GError function can't be passed into signals, or be used to
report error situations e.g. when setting object properties.
the three fields "domain", "code" and "blurb" could easily be stored
in a GValue structure which would be identified throguh a new fundamental
G_TYPE_ERROR.

> void     g_error_free          (GError        *error);

this always strikes me in the GLib API, we don't have consistent
lifetime API pairs such as:

new    + delete
alloc  + free
create + destroy

instead people mix them interchangably.

> GError*  g_error_copy          (const GError  *error);

if you want to allocate and copy errors on the fly, you should probably
use _alloc (or _init) and free. but since you already provide means for
duplicating errors (supposedly to pass them along?), what's wrong with
_new() + ref() & unref() which is probably the most consistent interface
we provide.

also i'm so much in fond of proving printf semantics on the default
constructor, if you offer printf() semantics, you better make that explicit
in the API:

GError*  g_error_new           (GQuark         module,
                                gint           code,
                                gpointer       data,
                                GDestroyNotify dnotify);
GError*  g_error_new_printf    (GQuark         module,
                                gint           code,
                                gpointer       data,
                                GDestroyNotify dnotify,
                                const gchar   *blurb,
                                ...) G_GNUC_PRINTF (...);


> /* if (err) *err = g_error_new(module, code, format, ...), also has
>  * some sanity checks.
>  */
> void     g_error_set           (GError       **err,
>                                 GQuark         module,
>                                 gint           code,
>                                 const gchar   *format,
>                                 ...) G_GNUC_PRINTF (4, 5);
> 
> void     g_error_set_with_data (GError       **err,
>                                 GQuark         module,
>                                 gint           code,
>                                 gpointer       data,
>                                 GDestroyNotify dnotify,
>                                 GDataCopyFunc  copyfunc,
>                                 const gchar   *format,
>                                 ...) G_GNUC_PRINTF (7, 8);
> 
> /* if (err && *err) { g_error_free(*err); *err = NULL; } */
> void     g_error_clear         (GError       **err);

oh hell, what's this about? ;)

> 
> 
> #ifdef __cplusplus
> }
> #endif
> 
> #endif /* GERROR_H */

---
ciaoTJ