Re: Issue 4365: Add ly_is_smob<T>(S) to check if S is a smob of type T. (issue 236850043 by address@hidden)

[nine . fierce . ballads]

On 2015/05/02 14:28:11, dak wrote:
> On 2015/05/02 12:54:18, Dan Eble wrote:
> > There might be some cases where ly_is_smob<Child>() is called where
only
> > ly_is_smob<Parent>() is required (though this seems unlikely).  The
right way
> to
> > improve performance in those cases is to ask the right question
instead.

> It becomes harder to ask the right question if we hide the difference
between
> right and wrong away.

I see your point.  To make it obvious when dynamic_cast occurs and to
reduce the number of ways of doing things, what do you think of
replacing both T::unsmob() and T::is_smob() with these?

  ly_unsmob<T> (s) which compiles iff T is the base smob type
  ly_dynamic_unsmob<T> (s) which incorporates a dynamic_cast

Or if you prefer the very long, very obvious way, dynamic_cast<Derived
*> (ly_unsmob<T> (s)) instead of the latter.

> So how is the intent of ly_is_smob clear to anyone not reading its
definition?

I don't think ly_is_smob<T> in context has any advantage over
T::is_smob; actually, T::is_smob looks nicer.  The problem is the
surprises that arise when inheritance is involved.  If T::is_smob()
didn't require the intentional overrides to avoid the surprises, it
wouldn't be worth changing.

> The question is whether one can throw in a mechanism for making
LY_ASSERT_SMOB
> deliver useful messages for such derived classes.
...
> Some side-channel advantage like that would go a lot towards
motivating such a
> change: the type testing for the callbacks in issue 4357 is pretty
awkward.

I disagree that fixing the inheritance problem with is_smob() needs any
more justification, but I will try to spend some time learning about
this other need.  Thanks for the feedback so far.

https://codereview.appspot.com/236850043/