Issue 4365: non-member unsmob<T> etc.

Add non-member unsmob<T> to replace T::unsmob and T::is_smob.

Hide Smob_base<>::is_smob() to prevent the misleading situation of
D::is_smob(S) returning true when S represents an ancestor of D rather
than a D.  This issue was previously worked around by overriding
is_smob in several derived smob classes; that is no longer needed.

LY_ASSERT_SMOB() now returns a smob pointer.  This is not used yet, but is available for future use to avoid unsmobbing twice.

Smob_base<>::smob_p() is hidden because there doesn't seem to be a
need for it to be public.

Files in lily/include were edited by hand.  Files in lily itself were
processed with the following sed script.

s/\([A-Za-z_]\+\)::derived_unsmob/unsmob<\1>/g
s/derived_unsmob *<\([A-Za-z_]\+\)>/unsmob<\1>/g
s/\([A-Za-z_]\+\)::unsmob/unsmob<\1>/g
s/\([A-Za-z_]\+\)::is_smob/unsmob<\1>/g
s/Input \*\([A-Za-z_]\+\) = unsmob /Input *\1 = unsmob<Input> /g
s/LY_ASSERT_SMOB (\([^)]*\))$/LY_ASSERT_SMOB (\1);/g

This is an evolution of the idea originally proposed in https://codereview.appspot.com/236850043/

[dak, 2015-05-13 11:20:54]

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

https://codereview.appspot.com/231680043/diff/1/lily/include/smobs.hh#newcode102
lily/include/smobs.hh:102: - unsmob<T> (SCM x) and derived_unsmob<T> (SCM x) -
unpack X and
My gut feeling is that it does not make sense to maintain separate unsmob<T> and
derived_unsmob<T> functions:  derived_unsmob<T> made sense in contrast to
T::unsmob (which could have produced a base unsmob in contrast).

https://codereview.appspot.com/231680043/diff/1/lily/include/smobs.hh#newcode163
lily/include/smobs.hh:163: typedef Super super_type;
That one's a nuisance.  Is it really necessary?

https://codereview.appspot.com/231680043/diff/1/lily/include/smobs.hh#newcode267
lily/include/smobs.hh:267: return is_smob<T> (s) ?
T::super_type::unchecked_unsmob (s) : 0;
This will fail to compile for

  return is_smob<T> (s) ? T::unchecked_unsmob (s) : 0;

anyway in case of derivation because of the missing upcast on the return type,
won't it?  No need for the super_type here, right?

https://codereview.appspot.com/231680043/diff/1/lily/include/smobs.hh#newcode280
lily/include/smobs.hh:280: return derived_unsmob<T> (s);
You can still put

  return T::is_smob (s) && derived_unsmob<T> (s);

here (assuming proper friendship) and bypass the dynamic_cast when the first
condition is false.

https://codereview.appspot.com/231680043/diff/1/lily/include/smobs.tcc
File lily/include/smobs.tcc (right):

https://codereview.appspot.com/231680043/diff/1/lily/include/smobs.tcc#newcod...
lily/include/smobs.tcc:159: ly_add_type_predicate ((void *) is_base_smob,
smob_name_.c_str ());
Why use is_base_smob here instead of is_smob<Super> ?  The whole point of
ly_add_type_predicate is to be able to identify type check functions by address,
and the address used in code elsewhere will be that of is_smob<Super>, won't it?

[Dan Eble, 2015-05-13 22:49:34]

https://codereview.appspot.com/231680043/diff/1/lily/include/lily-guile-macro...
File lily/include/lily-guile-macros.hh (right):

https://codereview.appspot.com/231680043/diff/1/lily/include/lily-guile-macro...
lily/include/lily-guile-macros.hh:252: ly_wrong_smob_arg
(is_derived_smob<klass>, var, number, __FUNCTION__); \
Oops. Checking for the base smob type was intended here.  Expect a new patch
addressing this and many of David's comments.

[Dan Eble, 2015-05-14 01:43:49]

On 2015/05/13 11:20:54, dak wrote:
> My gut feeling is that it does not make sense to maintain separate unsmob<T>
and
> derived_unsmob<T> functions:  derived_unsmob<T> made sense in contrast to
> T::unsmob (which could have produced a base unsmob in contrast).

I'll revert it to Base::unsmob and derived_unsmob<Derived>.

> lily/include/smobs.hh:163: typedef Super super_type;
> That one's a nuisance.  Is it really necessary?

Well, there is a difference in gcc's errors when is_smob<Derived> is tried.

When is_smob<T> is implemented as
    return Smob_base<T>::is_base_smob (s);
then the (ridiculous, voluminous) errors consist of reasons that
Smob_base<T> can not be instantiated.

When is_smob<T> is implemented as
    return Smob_base<typename T::super_type>::is_base_smob (s);
then the errors refer to this line and say that is_base_smob is private.
Since this line is introduced by a "this intentionally fails" comment,
I prefer this way, but not enough to fight for it.

I'm planning to leave this as it is until you confirm that you want me to
remove the typedef.

>   return T::is_smob (s) && derived_unsmob<T> (s);
> 
> here (assuming proper friendship) and bypass the dynamic_cast when the first
> condition is false.

dynamic_cast<T>(0) seems to be pretty fast so I think we should just stick with
the simpler implementation.  I timed both implementations, twice compiling a
single large score, and twice compiling a bunch of smaller scores, and I could
draw no conclusion from the results.

> lily/include/smobs.tcc:159: ly_add_type_predicate ((void *) is_base_smob,
> smob_name_.c_str ());
> Why use is_base_smob here instead of is_smob<Super> ?  The whole point of
> ly_add_type_predicate is to be able to identify type check functions by
address,
> and the address used in code elsewhere will be that of is_smob<Super>, won't
it?

I'll change it.  Thanks for the review.

[Dan Eble, 2015-05-14 01:48:37]

Allow T::unsmob again (etc.)

[dak, 2015-05-14 05:07:13]

On 2015/05/14 01:48:37, Dan Eble wrote:
> Allow T::unsmob again (etc.)

Sorry, I must be really bad at being comprehensible.  Either that, or there are
difficulties with my suggestion.  What I was suggesting was to get back to your
original plan of only having one function unsmob<T> (which you called
ly_unsmob<T> in your first patch) instead of differentiating between
derived_unsmob<T> and unsmob<T>.

The disadvantage with that is that we are going to sacrifice the performance
advantage of is_smob over unsmob for "base smobs".  The difference is not tied
to the redundant dynamic_cast but rather to having to fetch the pointer at all
(going through unsmob checks _both_ the type of the smob as well as its pointer
being non-zero.  The latter is always the case but the compiler does not know
that).

I also seem to remember using is_smob+unsmob (typical for use of LY_ASSERT_SMOB)
only fetched the type once whereas unsmob+unsmob fetched type, pointer, type,
pointer.  Maybe we should create some version of LY_ASSERT_SMOB leaving the
fetched pointer in a variable.  After all, that's the most frequent use, and
then this consideration of avoiding a double unsmob would be moot, again making
maintaining a separate is_smob in addition to a generic unsmob less attractive.

[dan_faithful.be, 2015-05-14 12:19:13]

> On May 14, 2015, at 01:07 , dak@gnu.org wrote:
> 
> On 2015/05/14 01:48:37, Dan Eble wrote:
>> Allow T::unsmob again (etc.)
> 
> Sorry, I must be really bad at being comprehensible.  Either that, or
> there are difficulties with my suggestion.  What I was suggesting was to
> get back to your original plan of only having one function unsmob<T>
> (which you called ly_unsmob<T> in your first patch) instead of
> differentiating between derived_unsmob<T> and unsmob<T>.

Oh.  The trouble was in communication.  I agree with you.  From the perspective
of someone who tries to work on the C++ side of Lilypond without having achieved
a comprehensive understanding of it, I would like having just unsmob<>.

> The disadvantage with that is that we are going to sacrifice the
> performance advantage of is_smob over unsmob for "base smobs".  The
> difference is not tied to the redundant dynamic_cast but rather to
> having to fetch the pointer at all (going through unsmob checks _both_
> the type of the smob as well as its pointer being non-zero.  The latter
> is always the case but the compiler does not know that).

Some template trickery could be used to select the more efficient implementation
for the type.  I’ll leave a summary comment in the new unsmob<>.

> I also seem to remember using is_smob+unsmob (typical for use of
> LY_ASSERT_SMOB) only fetched the type once whereas unsmob+unsmob fetched
> type, pointer, type, pointer.  Maybe we should create some version of
> LY_ASSERT_SMOB leaving the fetched pointer in a variable.

Something like this?

template <class T>
inline T* noisy_unsmob (la la la)
{
  // test here; return smob
}

#define LY_ASSERT_SMOB(klass, var, number) noisy_unsmob<klass> (var, number,
__FUNCTION__)

>  After all,
> that's the most frequent use, and then this consideration of avoiding a
> double unsmob would be moot, again making maintaining a separate is_smob
> in addition to a generic unsmob less attractive.

So in conclusion,
* use just unsmob<T> -- no is_smob<T> or derived_...<T>
* treat LY_ASSERT_SMOB specially for performance (test it)
* create a ticket suggesting a return value for LY_ASSERT_SMOB

Let me know if I got that wrong.  Thanks.
— 
Dan

[dak, 2015-05-14 17:56:46]

Dan Eble <dan@faithful.be> writes:

> On May 14, 2015, at 01:07 , dak@gnu.org wrote:
>
>> The disadvantage with that is that we are going to sacrifice the
>> performance advantage of is_smob over unsmob for "base smobs".  The
>> difference is not tied to the redundant dynamic_cast but rather to
>> having to fetch the pointer at all (going through unsmob checks _both_
>> the type of the smob as well as its pointer being non-zero.  The latter
>> is always the case but the compiler does not know that).
>
> Some template trickery could be used to select the more efficient
> implementation for the type.  I’ll leave a summary comment in the new
> unsmob<>.

Template trickery tends to be a maintenance pig.

>> I also seem to remember using is_smob+unsmob (typical for use of
>> LY_ASSERT_SMOB) only fetched the type once whereas unsmob+unsmob fetched
>> type, pointer, type, pointer.  Maybe we should create some version of
>> LY_ASSERT_SMOB leaving the fetched pointer in a variable.
>
> Something like this?
>
> template <class T>
> inline T* noisy_unsmob (la la la)
> {
>   // test here; return smob
> }
>
> #define LY_ASSERT_SMOB(klass, var, number) noisy_unsmob<klass> (var,
> number, __FUNCTION__)

I did not think of an actual return (and noisy_unsmob seems like an
absurd name, it's more like an assertive_unsmob or so, but it's not like
the name would be overly visible anyway) but that might actually be a
useful way of implementing it.  So one would do something like

Engraver *e = LY_ASSERT_SMOB(Engraver, eng, 1);

My idea would have been something like

LY_ASSERT_SMOB(Engraver, e, eng, 1)

but then it's not really clear whether you want to create e or only
assign to it.  Your proposal is probably quite cleaner and clearer and
it would seem backward-compatible (unless we get obnoxious warnings
about discarded return values).

>> After all, that's the most frequent use, and then this consideration
>> of avoiding a double unsmob would be moot, again making maintaining a
>> separate is_smob in addition to a generic unsmob less attractive.
>
> So in conclusion,
> * use just unsmob<T> -- no is_smob<T> or derived_...<T>
> * treat LY_ASSERT_SMOB specially for performance (test it)
> * create a ticket suggesting a return value for LY_ASSERT_SMOB

Unless GCC breaks out into warnings, one can probably just add the
return value right away.  And then make it a big ticket to actually make
use of it everywhere.  That will likely not work just with sed patterns.

> Let me know if I got that wrong.  Thanks.

Looks ok.

-- 
David Kastrup

[Dan Eble, 2015-05-15 21:08:02]

unsmob<T> everywhere (no perf testing yet)

[Dan Eble, 2015-05-16 12:59:47]

On 2015/05/15 21:08:02, Dan Eble wrote:
> unsmob<T> everywhere (no perf testing yet)

Here are results of processing a bunch of my own scores and a large score from
mutopia before (commit e391e47d29945653870cde23b91f487c0106a67d) and after these
changes.  I don't see any consistent differences.

before, multiple scores

  real    2m49.090s
  user    2m33.430s
  sys     0m14.109s

  real    2m47.874s
  user    2m32.962s
  sys     0m13.897s

  real    2m48.242s
  user    2m32.906s
  sys     0m14.317s

before, kv250 (mutopia)

(first run not timed)

  real    1m52.347s
  user    1m49.375s
  sys     0m2.856s

  real    1m50.517s
  user    1m47.291s
  sys     0m3.068s

unsmob<T>, multiple scores

  real    2m47.885s
  user    2m32.810s
  sys     0m14.085s

  real    2m48.601s
  user    2m33.986s
  sys     0m13.577s

  real    2m48.854s
  user    2m33.958s
  sys     0m13.853s

unsmob<T>, kv250 (mutopia)

  real    1m50.598s
  user    1m47.315s
  sys     0m3.108s

  real    1m51.382s
  user    1m48.203s
  sys     0m3.036s

  real    1m54.933s
  user    1m51.707s
  sys     0m2.984s

[dak, 2015-05-16 15:09:05]

https://codereview.appspot.com/231680043/diff/40001/lily/include/listener.hh
File lily/include/listener.hh (right):

https://codereview.appspot.com/231680043/diff/40001/lily/include/listener.hh#...
lily/include/listener.hh:185: unsmob<Callback_wrapper> (self)->trampoline_
(target, ev);
I don't really like this one.  "unsmob (self)" should really be "this".  Using
the even less specific unsmob<Callback_wrapper> seems like a step in the wrong
direction.  This might call for unchecked_unsmob (should this be protected
rather than private?).  It would be nicer to let LY_DECLARE_SMOB_PROC create its
own trampolining like Callback_wrapper does (wait, this *is* Callback_wrapper),
but I've not yet come up with a working scheme yet.

[Dan Eble, 2015-05-16 20:30:18]

Call unchecked_unsmob() where validity is assured

[Dan Eble, 2015-05-16 20:31:16]

Use unchecked_unsmob() where validity is assured

[Dan Eble, 2015-05-16 20:37:28]

On 2015/05/16 15:09:05, dak wrote:
> I don't really like this one.  "unsmob (self)" should really be "this".  Using
> the even less specific unsmob<Callback_wrapper> seems like a step in the wrong
> direction.  This might call for unchecked_unsmob (should this be protected
> rather than private?).

That seems pretty safe.  It would be mighty reckless to call "unchecked"
anything without understanding it, so I doubt it will be misused.

> It would be nicer to [...] 
> but I've not yet come up with a working scheme yet.

And sadly, I must decline to help.  Allergy medicine has cut my brainpower
today.

[Dan Eble, 2015-05-16 23:58:39]

rebase

[dak, 2015-05-17 12:49:54]

On 2015/05/16 20:37:28, Dan Eble wrote:
> On 2015/05/16 15:09:05, dak wrote:
> > I don't really like this one.  "unsmob (self)" should really be "this". 
Using
> > the even less specific unsmob<Callback_wrapper> seems like a step in the
wrong
> > direction.  This might call for unchecked_unsmob (should this be protected
> > rather than private?).
> 
> That seems pretty safe.  It would be mighty reckless to call "unchecked"
> anything without understanding it, so I doubt it will be misused.
> 
> > It would be nicer to [...] 
> > but I've not yet come up with a working scheme yet.
> 
> And sadly, I must decline to help.  Allergy medicine has cut my brainpower
> today.

Well ok, I managed.  Issue 4400.  I like that better than before anyway.

[dak, 2019-06-05 13:33:20]

https://codereview.appspot.com/231680043/diff/100001/lily/lily-guile.cc
File lily/lily-guile.cc (right):

https://codereview.appspot.com/231680043/diff/100001/lily/lily-guile.cc#newco...
lily/lily-guile.cc:459: // unsmob<T> delivers true.  This means that unsmob<T>
is a
The resulting comment text is nonsensical.  That might have been a clue that the
resulting behavior is not up to par, either.

[dan_faithful.be, 2019-06-06 03:21:36]

On Jun 5, 2019, at 09:33, dak@gnu.org wrote:
> 
> The resulting comment text is nonsensical.  That might have been a clue
> that the resulting behavior is not up to par, either.

I took a new job last October and it doesn’t leave me with much energy to dig
into LilyPond issues.
— 
Dan

[dak, 2019-06-06 07:42:16]

Dan Eble <dan@faithful.be> writes:

> On Jun 5, 2019, at 09:33, dak@gnu.org wrote:
>> 
>> The resulting comment text is nonsensical.  That might have been a clue
>> that the resulting behavior is not up to par, either.
>
> I took a new job last October and it doesn’t leave me with much energy
> to dig into LilyPond issues.

I hope that's good.  I'll take a look about what to do here.

-- 
David Kastrup