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 08:25:09, dak wrote:

> Nope.  dynamic_cast is non-trivial operation that cannot be optimized
away at
> compile time for almost any use.  Calling it for every instance of the
rather
> ubiquitous is_smob query (ubiquitous enough that I made it an
operation separate
> from unsmob and checked that the generated code was nicer) is
excessive.

When "Patchy the autobot says: passes tests" does that include any
profiling?

Since you've been through the exercise of examining generated code, I'm
curious whether you found dynamic_cast<T*>(T*) (or any upcast) producing
anything worse than static_cast<T*>(T*) would have.  That would shock
me; but if there is some compiler version actually in use that is that
oblivious, we should be able to adapt a template metaprogramming trick
from boost or C++11 that avoids the dynamic_cast in those cases.

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.

In the case of necessary down-casting, a test is required to prevent the
function from answering the wrong question.  There is evidence that this
has been recognized as a problem in some cases already: Engraver and
other classes have an overridden unsmob() that includes a dynamic cast
and an overridden is_smob() that depends on it.  Are there other cases
waiting to be discovered?  Is somebody going to contribute one next week
that will be discovered months from now?  This patch would make it
possible to answer "No, ly_is_smob<T>(s) returns true if and only if s
is a T."

> And all that for avoiding less of a dozen instances of an open-coded
> dynamic_cast with reasonably clear intent?  No.

The intent of overriding Engraver::is_smob() etc. is clear only to one
who bothers to read it.  Maybe you undervalue avoiding future misuse
because of talent and familiarity.

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