Reviewers: carl.d.sorensen_gmail.com,
Message:
My intent was to make it clear to the person using these objects what
is going on.
One thing that was hidden was the extra cost of getting an Item* over
getting a Grob*. Consider this excerpt from the old version of
Dynamic_align_engraver:
create_line_spanner (info.grob ());
if (has_interface<Spanner> (info.grob ()))
{
started_.push_back (info.spanner ());
current_dynamic_spanner_ = info.spanner ();
}
Perhaps the person who wrote this code understood but didn't care that
he asked three times (at least: I haven't looked into
create_line_spanner) for a runtime check that info refers to a
Spanner. I think it is more likely that he did not see or had
forgotten about the cast. If he hadn't had info.spanner () available
to him, and had had to type out dynamic_cast<Spanner*> (info.grob ()),
he probably would have structured the code to do it once, not three
times.
To be clear, the cost of dynamic casting in lilypond is not huge; but
it is noticeable and it can be improved. Last week I profiled one of
the rest regression tests that always shows differences and found that
1% of cycles are spent on dynamic casting. I think that's 1% of score
processing, but it might be 1% of the total; I'm not sure because it
was my first time using the tool and I chose to move on to another
task before verifying which.
Point two:
Ligature_bracket_engraver::acknowledge_note_column (Grob_info info)
{
if (ligature_)
{
Tuplet_bracket::add_column (ligature_,
info.item ());
add_bound_item (ligature_, info.item ());
}
}
If info refers to a Spanner, then this code passes a null pointer to
add_column and add_bound_item. I'm not sure if that would be bad, but
let's assume it would. If the person who coded this had had to write,
Item *item = dynamic_cast<Info*> (info.grob ());
... (item);
... (item);
he would be more likely to notice that the pointer might be null and
handle it.
But now let's say that we know that it is impossible for
acknowledge_note_column to be passed a Grob_info referring to anything
but an Item. (That might actually be the case, but I'm not sure.)
Well, that's checkmate for Grob_info::item (), because all that is
required to handle _that_ is static_cast, and _it_ doesn't have the
overhead of a function call and a dynamic_cast.
Along the same lines are things like the following. I hope I can
describe this as "loose" without offending any of my worthy comrades
here:
int
Paper_column::get_rank (Grob const *me)
{
return dynamic_cast<Paper_column const *> (me)->rank_;
}
If "me" is a valid Grob that is not a Paper_column, this dereferences
a null pointer, which is not something that any function should be
doing, especially not one with an interface that accepts a Grob. A
function that will not work with anything but a Paper_column should
have Paper_column in its signature.
Well, my kids have come home! I have to go, but I think that's about
all I have to write anyway. I'll add more later if anything comes to
mind.
Description:
https://sourceforge.net/p/testlilyissues/issues/5336/
Presenting dynamic casts as simple getters was hiding something that
is better left in the open.
Please review this at https://codereview.appspot.com/344010043/
Affected files (+57, -73 lines):
M lily/break-align-engraver.cc
M lily/clef-engraver.cc
M lily/cue-clef-engraver.cc
M lily/dynamic-align-engraver.cc
M lily/extender-engraver.cc
M lily/grob-array.cc
M lily/grob-info.cc
M lily/hyphen-engraver.cc
M lily/include/grob-array.hh
M lily/include/grob-info.hh
M lily/ligature-bracket-engraver.cc
M lily/ottava-engraver.cc
M lily/paper-column-engraver.cc
M lily/piano-pedal-align-engraver.cc
M lily/pointer-group-interface.cc
M lily/pure-from-neighbor-engraver.cc
M lily/separating-line-group-engraver.cc
M lily/spacing-interface.cc
M lily/spanner-break-forbid-engraver.cc
M lily/tab-tie-follow-engraver.cc
M lily/volta-engraver.cc
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
