Re: srfi-18 requirements

[Julian Graham]

Hi Neil,

> Looking good!  Many thanks for your continuing work on this, and sorry
> for my delay (once again!) in reviewing.  I have a few comments, as
> follows.

No worries.  Find my responses below.


> >  @c begin (texi-doc-string "guile" "join-thread")
> > address@hidden {Scheme Procedure} join-thread thread
> > address@hidden {Scheme Procedure} join-thread thread [timeout]
> >  @deffnx {C Function} scm_join_thread (thread)
> > address@hidden {C Function} scm_join_thread_timed (thread, timeout)
>
> Didn't we agree to add a timeout-val parameter here?

No, we didn't, although I agree such a parameter would be pretty
useful.  I'll add that in the next revision I send you.


> > +static scm_t_timespec
> > +scm_to_timespec (SCM t)
>
> For static functions it's nice to omit the scm_ prefix, because they
> don't need it, and it makes it clearer to the casual reader that
> they're not part of the API.
>
> Also, can the signature be void to_timespec (SCM t, scm_t_timespec *),
> to avoid relying on support for struct return?

Yes and yes.


> > +       else if (!first_iteration)
> > +         {
> > +           if (timeout != NULL)
> > +             {
> > +               gettimeofday (&current_time, NULL);
> > +               if (current_time.tv_sec > timeout->tv_sec ||
> > +                   (current_time.tv_sec == timeout->tv_sec &&
> > +                    current_time.tv_usec * 1000 > timeout->tv_nsec))
> > +                 {
> > +                   *ret = 0;
> > +                   break;
> > +                 }
>
> Is timeout an absolute time, or relative to when join-thread was
> called?  Before getting to this code, I thought it was relative - but
> then I don't see how the code above can be correct, because it is
> comparing against the absolute gettimeofday() ...?

It's absolute -- like the arguments for the existing timed
synchronization primitives (and like the timed parts of the SRFI-18
API).  (Unless I'm mistaken...)


> > -static char *
> > -fat_mutex_unlock (fat_mutex *m)
> > +static void
> > +fat_mutex_unlock (SCM mx)
> >  {
> > -  char *msg = NULL;
> > -
> > +  fat_mutex *m = SCM_MUTEX_DATA (mx);
> >    scm_i_scm_pthread_mutex_lock (&m->lock);
> > -  if (!scm_is_eq (m->owner, scm_current_thread ()))
> > +  if (m->level > 0)
> > +    m->level--;
> > +  else
>
> It looks like there is a significant change to the semantics here: any
> thread can unlock a mutex, not just the thread that locked it.  Is
> that the intention, or am I misunderstanding?

No, that's the intention (it's explicitly permitted by SRFI-18).  I
thought you were okay with that, since it was not on your list of
stuff that didn't belong in C.  If that's too big of a change, might I
suggest we add a function that forcibly unlocks a mutex, regardless of
the owner?


> Actually, that strongly says to me that we don't need the `cond' part
> of this API to be implemented in C.  Can we move that to the SRFI-18
> Scheme code, and leave the C API as a plain unlock-mutex operation?

Fine by me (again. left this one in because you didn't squawk about it
earlier), except that it might be harder to guarantee the safety of
mixing the mutex and cond passed to the SRFI-18 Scheme implementation
with non-SRFI-18 calls -- C generally provides a convenient protection
against deadlock for things like that.


Regards,
Julian