Re: gerror.c revisited

[Sebastian Wilhelmi <wilhelmi ira uka de>]

Hi Owen,

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

Ok, but then I would like to have something like static quarks.
(Introducing a GStaticQuark, while technically better seems like WAY too much
overhead, but that one function here seems to help, especially considering the
MT-issues of the code.)

glib.h:
-------

GQuark g_quark_from_static_string_once (GQuark *quark, const gchar* string);

extern GQuark g_thread_error_quark; 
extern const gchar* g_thread_error_string;
#define G_THREAD_ERROR g_quark_from_static_string_once
(&g_thread_error_quark,  
                                                     &g_thread_error_string);

gthread.c:
----------
GQuark g_thread_error_quark = 0;
const gchar* g_thread_error_string = "gthread-error";

gdataset.c:
-----------
GQuark 
g_quark_from_static_string_once (GQuark *quark, const gchar* string)
{
  G_LOCK_DEFINE_STATIC(lock);
  if (!*quark)
  {
    G_LOCK (lock);
    if (!*quark)
      *quark = g_quark_from_static_string (string);
    G_UNLOCK (lock);
  }
  return *quark;
}

The double check for '!*quark' is necessary to prevent race conditions. A
locking around the outer 'if' is not necessary tough, because the quarks just
represent themselves and are not pointing to other memory, so even in presence
of incoherent cache it should work.

An alternative is to make a function in gthread.c instead of the extern
variables in glib.h above:

GQuark 
g_thread_error_quark
{
  GQuark quark;
  return quark ? quark : g_quark_from_static_string_once (&quark,
                                                         "gthread-error");
}

That would help keeping the variable name space of glib sparse.

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

That of course is quite a convincing argument.

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

Ok.
 
> >  {
> > +  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).

Yes, this will get removed. It was just a quick hack to try to see, what 
happens. It won't get committed.

So for now I'm going to commit the uncontroversial items (g_propagate_error
and my gthread error extension) and wait for your opinions regarding the
"static quarks".

Bye,
Sebastian
-- 
Sebastian Wilhelmi                   |            här ovanför alla molnen
mailto:wilhelmi@ira.uka.de           |     är himmlen så förunderligt blå
http://goethe.ira.uka.de/~wilhelmi   |