Re: gerror.c revisited

[Owen Taylor <otaylor redhat com>]

Sebastian Wilhelmi <wilhelmi@ira.uka.de> writes:

> [resent to the list, which I forgot the first time, only mailing Havoc]
> Hi Havoc, 
> 
> > I'm doing it this way in gexec.h:
> >  #define G_EXEC_ERROR g_exec_error_quark ()
> > 
> >  GQuark g_exec_error_quark();
> > 
> > Then the convention for error codes is G_EXEC_ERROR_* members in
> > GExecErrorType.
> > 
> > I guess since you already can't use switch(), it may as well be a
> > string. That makes sense to me. Anyone see a problem with it?
> 
> The attached patch implements this. I've g_strduped the domain. It should
> however be possible to just assign it. But the copy-solution avoids stupid
> surprises (for lets say unloaded modules or the like).

Hmmm, I actually don't agree here. I think that the quark works
better:

 - It's (marginally) more efficient, since you don't have duplication
   of the string constant, and you avoid string compares.

 - It allows code like:

   if (err.domain == G_THREAD_ERROR)
     {
       switch (err.code)
        {
          [...]
        } 
     }

    which is clearer than;

   if (strcmp (err.domain, G_THREAD_ERROR) != 0)
     {
     }

I don't see any real compelling advantages for the string, except
that it is a tiny bit less work to define a new error domain.

(Note that the log domain is a string printed to the user,
while this is not (the message is supposed to be self-contained.),
so I don't think the parallel holds.)

I've convinced Havoc back to this position, so I guess he doesn't
have strong views on the matter.


> > > +#define glib_error_domain() g_quark_from_static_string ("glib-error-domain")
> > > +#define GLIB_ERROR_AGAIN   1   /* Resource temporarily unavailable */
> > > +
> > 
> > I would do this as G_THREAD_ERROR (for the domain) and an enum
> > GThreadErrorType with member G_THREAD_ERROR_AGAIN. The convention is
> > debatable, but we should use the same one throughout glib.
> 
> Ok, done.

Actually, the convention I agreed on with Havoc was GThreadError,
not GThreadErrrorType.
 
> Yes, but this code would work, if g_set_error would exit for NULL.
> 
> > GError *local_err = NULL;
> > foo (&local_err);
> > if (local_err != NULL)
> > {
> >   if (err)
> >    *err = local_err;
> >   else
> >    g_error_free (local_err);
> > 
> >   return NULL;
> > }
> 
> This last part looks like it will be used in a whole lot of functions  and it
> seems a bit tedious and easy to get wrong. So I included g_propagate_error for
> that purpose, what do you think?

I think g_propagate_error() is a good thing, though I'm not completely
convinced that returning the boolean is a good thing:

 if (g_propagate_error (err, local_err))
   return NULL;

is distinctly less clear than:
   
 if (local_err != NULL)
   {
     g_propagate_error (err, local_err))
     return NULL;
   }

Also, the second avoids a function call in the non-error case.

> +void     
> +g_propagate_error (GError       **dest,
> +		   GError        *src)
> +{

You should add g_return_if_fail (src != NULL) here - src == NULL
just doesn't make sense.

> +  if (dest) 
> +    {
> +      if (*dest != NULL)
> +	g_warning ("GError set over the top of a previous GError or uninitialized memory.\n"
> +		   "This indicates a bug in someone's code. You must ensure an error is NULL before it's set.");      
> +      *dest = src;
> +    }
> +  else if (src) 

See above.

> +    g_error_free (src);

>  {
> +  int i;
> +  GError *err = NULL;
> +  g_thread_init (NULL);
>    /* Only run the test, if threads are enabled and a default thread
>       implementation is available */
> +  for (i = 0; i < 10000; i++)
> +    {
> +      g_thread_create (test_g_static_rw_lock_thread, 
> +		       0, 0, TRUE, TRUE, 
> +		       G_THREAD_PRIORITY_NORMAL, &err);      
> +      g_print ("%d\n",i);
> +      if (err)
> +	g_error ("%s, %d, %s.\n", err->domain, err->code, err->message);

 1) err->message should be self contained for printing out.
 2) dumping core here really is not appropriate. 

  g_warning ("%s\n", err->message);
  exit(1);

(We should really add g_fatal() that prints a message and does exit(1)
without the abort).

Regards,
                                        Owen