Re: GError implementation

[Havoc Pennington <hp redhat com>]

Tim Janik <timj@gtk.org> writes: 
> i already said this to owen, do s/module/domain/ to saty consistent with
> the g_log() naming scheme and avoid confusion with GModules.
>

OK.
 
> >   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".
> 

"str" is an established convention in glib, and "string" clashes with
C++ in annoying ways (though it does compile, for example Emacs will
syntax-highlight it specially). So I don't like it as much as str.

> >   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?

Because you can write this:

GError *err = NULL;
frobate (&err);
if (g_error_matches (*err, BLAH_ERROR, BLAH_ERROR_FOO))
  handle_error ();

otherwise you have to write this:

GError *err = NULL;
frobate (&err);
if (*err && g_error_matches (*err, BLAH_ERROR, BLAH_ERROR_FOO))
  handle_error ();

And that's an easy thing to get wrong, and pointless typing.

>  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.
> 

Right, I'll make it a function.
 
> huh? ok there's no GDestroyNotify function in GError, probably an
> omission,

It's in GRealError

> but what's that with GDataCopyFunc? where will you store that? 


GRealError

> and btw,
> you're pollution the already used GData namespace.

If we provide a long-term guarantee that GError is an immutable object
(i.e. we can't add g_error_set_*()) then we could use ref/unref. If we
reserve the right to make it mutable, we need a copy function.

If there's no way to copy or ref an error, people will get annoyed I'm
sure, so we need one or the other.
 
> > 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.
>

Owen said he liked _new and _free, so I've been using those for new
stuff. _new and _free are shorter than the others, and _free is
distinct from the special meaning of _destroy in GtkWidget.
 
> 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 (...);
>

I think the printf version will be used 95% of the time, so it's kind
of bad to make it long and inconvenient IMO.

Why not add a non-printf version:

  GError * g_error_new_literal    (GQuark, gint, const char *);
 
> 
> > /* 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? ;)
>

Just a convenience function...

g_error_set and g_error_clear aren't actually methods on GError, which
is a bit confusing, in GConf I call them gconf_set_error() etc. But I
don't like g_set_error () since it isn't in a glib sub-namespace...

Havoc