Re: gerror.c revisited

[Owen Taylor <otaylor redhat com>]

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

> > 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 though,
> because the quarks just represent themselves and are not pointing to
> other memory, so even in presence of incoherent cache it should
> work.

In my understanding:
 
 - if GQuark is not written atomically, you need a lock on
   the outside, around the read.

 - If GQuark is written atomically (a safe assumption), then
   no locking is necessary, since not only do quarks represent
   themself, but g_quark_from[_static]_string is idempotent.
   It does no harm to call it multiple times. So, the obvious
   implementation
 
====
   if (!*quark)
      *quark = g_quark_from_static_string (string);

   return *quark;
====

is actually thread safe. The worst thing that happens is that *quark
gets written to twice with the same value. Which makes me question the
need for g_quark_from_static_string_once() at all.

Instead of:

====

glib.h
------
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";

We can do the simpler, if slightly longer:

glib.h
------
#define G_THREAD_ERROR g_thread_error_quark ()
GQuark g_thread_error_quark (void);

gthread.c
---------
GQuark
g_thread_error_quark (void)
{
  static GQuark quark = 0;
  if (!quark)
    quark = g_quark_from_static_string ("gthread_error");

  return quark;
}

Which, as you say, has less exprted symbols. We could even have:

#define G_DEFINE_ERROR_QUARK(name)                  \
  GQuark                                            \
  name##_quark (void)                               \
  {                                                 \
    static GQuark quark = 0;                        \
    if (!quark)                                     \
      quark = g_quark_from_static_string (#name);   \
                                                    \
    return quark;                                   \
  }

to further shorten the .c file portion, if it makes it less
clear....

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

I don't see any advantage in this, considering my comments about
threads above.

Regards,
                                        Owen