Provide some more smob functionality and use it

Provide some more smob functionality and use it

This adds basic smob types Smob1, Smob2, Smob3 for Smobs with minimal
data requirements and no separate representation on the heap.

It also allows declaring smobs that can be called like functions.

Reimplemented smobs are Input, Musicfunction, Simple_closure,
Unpure_pure_containers and Unpure_pure_call.


The first upload is just the reference situation after issue4082 is
committed.  The next upload caters for this issue.

[dak, 2014-08-31 15:34:12]

The actual full changes on top of issue 4082

[lemzwerg, 2014-08-31 16:45:37]

LGTM.  Very nice!  This indeed feels more C++ now.

https://codereview.appspot.com/135240043/diff/20001/scripts/auxiliar/smob-con...
File scripts/auxiliar/smob-convert.sh (right):

https://codereview.appspot.com/135240043/diff/20001/scripts/auxiliar/smob-con...
scripts/auxiliar/smob-convert.sh:2: for i in $(git grep -l
'^\s\+DECLARE_SIMPLE_SMOBS' lily)
Will this script be ever used again?  It perhaps deserves a comment...

[dak, 2014-08-31 17:03:10]

On 2014/08/31 16:45:37, lemzwerg wrote:
> LGTM.  Very nice!  This indeed feels more C++ now.

Well, it is sort of cheating C++.  In order to avoid polymorphism (virtual
functions, vtable, dynamic_cast etc) for the lightweight smobs, this uses
templates for a sort-of back-and-forward inheritance: the base smob class knows
its super class via a template parameter, and the super class knows its
base smob class via inheritance.  Overrides work via the base class accessing
Super::whatever which falls through to the base class unless defined in Super. 
So the code ends up somewhat similar to the macro code.  And the access of
Super::whatever has about the type safety of macro code as well.  Which is
convenient since "please ignore this by default" functions can be defined to be
constant zero and the access gets optimized away completely.

The end result is pretty much like "virtual overrides" with regard to the coding
style for the user but more memory- and code efficient.  And the total
light-weight smobs Smob1..Smob3 cheat by having no data members and misusing
"this" as an SCM value (SCM is defined as a pointer by default as well).  With
the result that source looks like with the non-lightweight smobs but the
generated code is close to non-existent.

>
https://codereview.appspot.com/135240043/diff/20001/scripts/auxiliar/smob-con...
> File scripts/auxiliar/smob-convert.sh (right):
> 
>
https://codereview.appspot.com/135240043/diff/20001/scripts/auxiliar/smob-con...
> scripts/auxiliar/smob-convert.sh:2: for i in $(git grep -l
> '^\s\+DECLARE_SIMPLE_SMOBS' lily)
> Will this script be ever used again?  It perhaps deserves a comment...

The script will not get used again except possibly by people having a large
amount of code in an unconverted branch.  The script will not make it outside of
the branch: its existence will get reverted away at the end.  If people need it,
they'll have to cherry-pick it out of the branch.  There is no real point in
retaining the script even though it's sort-of educational.  But it does not
apply to anything after having run once.

[eble, 2014-09-01 01:54:44]

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#new...
lily/include/smobs.hh:93: list = scm_cons (p->unprotect (), list);
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.

https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh#new...
lily/include/smobs.hh:158: // constructor, in constrast, may not be called at
all in classes
contrast

https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh#new...
lily/include/smobs.hh:225: return is_smob (s) ? SCM_BOOL_T : SCM_BOOL_F;
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;
}

https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh#new...
lily/include/smobs.hh:248: class Smob: public Smob_base<Super> {
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).

[dak, 2014-09-01 02:58:09]

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

[dak, 2014-09-01 03:35:43]

On 2014/09/01 02:58:09, dak wrote:
> https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh

>
https://codereview.appspot.com/135240043/diff/20001/lily/include/smobs.hh#new...
> 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.

Ok, I am pulling the "old code did not do this either" defense for good since I
get
In file included from ./include/listener.hh:64:0,
                 from ./include/translator-group.hh:23,
                 from audio-element-info.cc:22:
./include/smobs.hh: In copy constructor
'Translator_group::Translator_group(const Translator_group&)':
./include/smobs.hh:239:3: error: 'Smob<Super>::Smob(const Smob<Super>&) [with
Super = Translator_group]' is private
   Smob (const Smob<Super> &);
   ^
In file included from audio-element-info.cc:22:0:
./include/translator-group.hh:50:7: error: within this context
 class Translator_group : public Smob<Translator_group>
       ^
In file included from ./include/translator.hh:25:0,
                 from ./include/translator-group.hh:24,
                 from audio-element-info.cc:22:
./include/translator-group.hh: In member function 'virtual Translator_group*
Translator_group::clone() const':
../flower/include/virtual-methods.hh:45:27: note: synthesized method
'Translator_group::Translator_group(const Translator_group&)' first required
here 
     return new name (*this);                    \
                           ^
./include/translator-group.hh:70:3: note: in expansion of macro
'VIRTUAL_COPY_CONSTRUCTOR'
   VIRTUAL_COPY_CONSTRUCTOR (Translator_group, Translator_group);
   ^

So clearly making the copy constructor private is not a no-brainer.  The change
would actually be not in this issue but issue 4082 which is basically a
conversion issue from macros to templates.  Since squeezing in privatization of
constructors/assignment turns out to be tricky in itself, I'm not going to put
it in the same issue.  A separate commit, issue and testing seems called for and
I don't see a reason to stop the horses for this one if the trivial course does
not work.  Do you want to try your hand on this one once issue4082 is through?

[eble, 2014-09-01 04:13:41]

On 2014/09/01 03:35:43, dak wrote:
> A separate commit, issue and testing seems called for and
> I don't see a reason to stop the horses for this one if the trivial course
does
> not work.  Do you want to try your hand on this one once issue4082 is through?

I've been gearing up to contribute some part combiner improvements (IMO) that
have been gathering dust for 4-5 years.  If this new issue is still up for grabs
after I'm done with that, I'll be happy to try.

[dak, 2014-09-01 14:18:08]

Streamline Music_function, remove duplicate ly:input-location?

[janek, 2014-09-03 22:55:14]

Hi David,

I'm sorry that i didn't manage to review this, but from a quick look it seems
like a very welcome change.  I'm really getting an impression that Scheme-C++
interfacing in LilyPond becomes easier to understand thanks to your work.

best,
Janek