Critical section review

Critical section review

[Neil Jerram]

OK, to get the ball rolling and fill in my understanding: 
scm_make_memoized() in debug.c has:

   SCM_CRITICAL_SECTION_START;
   SCM_NEWSMOB (z, SCM_UNPACK (exp), SCM_UNPACK (env));
   SCM_NEWSMOB (ans, scm_tc16_memoized, SCM_UNPACK (z));
   SCM_CRITICAL_SECTION_END;

So:

- why do we need a critical section here?

- given that there is a critical section here, isn't there a problem 
that one of the SCM_NEWSMOBs could throw a memory error?

(I have read what it says in the manual on critical sections, by the way.)

Regards,
	Neil


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Critical section review

[Mikael Djurfeldt]

On Wed, 23 Mar 2005 08:16:00 +0000, Neil Jerram <neil@ossau.uklinux.net> wrote:
> OK, to get the ball rolling and fill in my understanding:
> scm_make_memoized() in debug.c has:
> 
>    SCM_CRITICAL_SECTION_START;
>    SCM_NEWSMOB (z, SCM_UNPACK (exp), SCM_UNPACK (env));
>    SCM_NEWSMOB (ans, scm_tc16_memoized, SCM_UNPACK (z));
>    SCM_CRITICAL_SECTION_END;
> 
> So:
> 
> - why do we need a critical section here?

We don't.

The above code is the result of multiple applications of query-replace
during several years of GUile development. All four lines should be
replaced by a call to SCM_RETURN_NEWSMOB.

M


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Critical section review

[Mikael Djurfeldt]

On Wed, 23 Mar 2005 08:16:00 +0000, Neil Jerram <neil@ossau.uklinux.net> wrote:
> OK, to get the ball rolling and fill in my understanding:
> scm_make_memoized() in debug.c has:
> 
>    SCM_CRITICAL_SECTION_START;
>    SCM_NEWSMOB (z, SCM_UNPACK (exp), SCM_UNPACK (env));
>    SCM_NEWSMOB (ans, scm_tc16_memoized, SCM_UNPACK (z));
>    SCM_CRITICAL_SECTION_END;

In addition, the first call to SCM_NEWSMOB is obviously bogus. It
should be a call to scm_cons.

M


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Critical section review

[Neil Jerram]

Mikael Djurfeldt wrote:
> On Wed, 23 Mar 2005 08:16:00 +0000, Neil Jerram <neil@ossau.uklinux.net> wrote:
> 
>>OK, to get the ball rolling and fill in my understanding:
>>scm_make_memoized() in debug.c has:
>>
>>   SCM_CRITICAL_SECTION_START;
>>   SCM_NEWSMOB (z, SCM_UNPACK (exp), SCM_UNPACK (env));
>>   SCM_NEWSMOB (ans, scm_tc16_memoized, SCM_UNPACK (z));
>>   SCM_CRITICAL_SECTION_END;
> 
> 
> In addition, the first call to SCM_NEWSMOB is obviously bogus. It
> should be a call to scm_cons.

Thanks.  So this is now just:
   SCM_RETURN_NEWSMOB (scm_tc16_memoized, scm_cons (exp, env));

which is a completely mechanical change except that one SCM_UNPACK(...) 
has become ((scm_t_bits) ...) - is the definition of SCM_NEWSMOB correct 
in using (scm_t_bits) rather than SCM_UNPACK ?

	Neil


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Critical section review

[Mikael Djurfeldt]

On Wed, 23 Mar 2005 20:19:22 +0000, Neil Jerram <neil@ossau.uklinux.net> wrote:
> So this is now just:
>    SCM_RETURN_NEWSMOB (scm_tc16_memoized, scm_cons (exp, env));
> 
> which is a completely mechanical change except that one SCM_UNPACK(...)
> has become ((scm_t_bits) ...) - is the definition of SCM_NEWSMOB correct
> in using (scm_t_bits) rather than SCM_UNPACK ?

Well,I guess so. I'm no SCM_PACK/SCM_UNPACK expert.

M


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Critical section review

[Marius Vollmer]

Neil Jerram <neil@ossau.uklinux.net> writes:

> Thanks.  So this is now just:
>    SCM_RETURN_NEWSMOB (scm_tc16_memoized, scm_cons (exp, env));
>
> which is a completely mechanical change except that one
> SCM_UNPACK(...) has become ((scm_t_bits) ...) - is the definition of
> SCM_NEWSMOB correct in using (scm_t_bits) rather than SCM_UNPACK ?

The definition of SCM_NEWSMOB is correct; its prototype is

  SCM SCM_NEWSMOB (scm_t_bits tag, scm_t_bits data);

However, if you want to use a SCM value as the data of a smob, you
need to use SCM_UNPACK to convert that SCM into a scm_t_bits:

  SCM_RETURN_NEWSMOB (scm_tc16_memoized, SCM_UNPACK (scm_cons (exp, env)));

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Critical section review

[Neil Jerram]

Marius Vollmer wrote:
> 
> The definition of SCM_NEWSMOB is correct; its prototype is
> 
>   SCM SCM_NEWSMOB (scm_t_bits tag, scm_t_bits data);

In this case, why does the implementation of SCM_NEWSMOB need a cast? 
Surely this cast is likely to hide bad uses of SCM_NEWSMOB.

> 
> However, if you want to use a SCM value as the data of a smob, you
> need to use SCM_UNPACK to convert that SCM into a scm_t_bits:
> 
>   SCM_RETURN_NEWSMOB (scm_tc16_memoized, SCM_UNPACK (scm_cons (exp, env)));

OK, I've done this now, although I think it would be more convenient to 
move the SCM_UNPACK into the definition of SCM_RETURN_NEWSMOB.

	Neil


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Critical section review

[Neil Jerram]

FYI, I've committed some review comments to eval.c and dynl.c (having 
skipped deprecated.c for now on the grounds that it's less important). 
You can find them by grepping for "njrev".

(Obviously, feel free to change or remove them after consideration.)

	Neil


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel