Re: Provide some more smob functionality and use it (issue 135240043 by address@hidden)

[dak]

https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh
File lily/include/smobs.hh (right):

https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh#newcode93
lily/include/smobs.hh:93: list = scm_cons (p->unprotect (), list);
On 2014/09/01 01:54:44, eble wrote:
> I'm ignorant of how Guile collects garbage, but hopefully it wouldn't
preempt
> this after p is unprotected and before scm_cons appropriates it.

GUILE scans the stack and registers for SCM values.  As long as you are
sticking with SCM for manipulations (rather than work only with the
unsmobbed pointer) and keep them in automatic storage, you are fine even
after unprotection.

Otherwise you couldn't even do things like
x = scm_sum (x, scm_from_int (1))
without running into danger that either the sum or 1 would be
unprotected in the middle of execution.  The stack scanning is mentioned
in the description of Simple_smob.

https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh#newcode158
lily/include/smobs.hh:158: // constructor, in constrast, may not be
called at all in classes
On 2014/09/01 01:54:44, eble wrote:
> contrast

Will do before committing.  Thanks.

https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh#newcode225
lily/include/smobs.hh:225: return is_smob (s) ? SCM_BOOL_T : SCM_BOOL_F;
On 2014/09/01 01:54:44, eble wrote:
> Wouldn't something like the following be useful here and elsewhere?
(For
> example, I found ly_grob_pq_less_p right away, and I wasn't even
trying hard.)
> SCM to_SCM_BOOL(bool b)
> {
>      return b ? SCM_BOOL_T : SCM_BOOL_F;
> }

scm_from_bool exists.  I don't know offhand whether it is a macro but
even if it is, for top-level idiomatic C expressions like this I see no
readability improvement.  As part of a more complex expression that
might be different.

https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh#newcode248
lily/include/smobs.hh:248: class Smob: public Smob_base<Super> {
On 2014/09/01 01:54:44, eble wrote:
> Are the compiler-generated copy constructor and assignment operator
going to do
> the right thing?  If so, a comment to that effect would assure us that
you
> considered them.  If not, they should either be implemented (if
useful) or
> declared but not defined (if not useful).

Indeed, a Smob (as opposed to Simple_smob and the Smob1... forms) should
only be handled via SCM and/or protected pointers.  private copy
constructors would make sense.  Will do and recheck.

In my defense, it's not like the old code did that either.

https://codereview.appspot.com/135240043/