Re: Issue 5620: Change vector<Grob *> to vector<Paper_column *> (issue 545280043 by address@hidden)

[nine . fierce . ballads]

On 2019/11/26 08:25:04, lemzwerg wrote:
> LGTM.  The goal is to have less dynamic casts, right?

Yes, and most of all, fewer cases like this:

    dynamic_cast<Paper_column *> (c[j])->set_system (system);

Dynamic casting is for casting to a derived type when the actual type of
the object is not known at compile time.  It returns a null pointer in
the case that casting is inappropriate.  If it returns a null pointer
and you dereference it, you have made a fatal error; so, one can say two
things about this example.  If the programmer knew that it is impossible
for c[j] to refer to anything other than a (or a kind of) Paper_column,
he should have used static_cast[1], which has a lower run-time cost.  If
he knew that it is possible for c[j] to refer to something else, then he
should have checked the pointer.

A maintainer coming across this has a choice.  A conservative maintainer
with limited time to investigate would probably choose to check the
pointer and slow down LilyPond.

My goal is to preserve more type information in the C++ domain so that
such questionable code is much less likely to exist.

[1] Even static_cast is not ideal, because the programmer's "knowledge"
might be an improperly justified, false belief; or it might have been
true at one time, in a context that has since changed.  It is better to
use the compiler's type checking as much as is practical.

P.S. This might have come across a bit ranty--it is--but I don't intend
to demean any LilyPond contributors, just to improve things.

https://codereview.appspot.com/545280043/