Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
On 2018/06/13 03:24:02, dan_faithful.be wrote: I perceive that we understand each other’s points and simply disagree. There is nothing new I want to counter with. I will just state that if a contributor were made uncomfortable by dynamic_cast, my two-pronged solution would be (1) gently encourage him to educate himself on this fundamental feature of C++, and (2) over time, rework the software to require fewer casts by preserving more type information in the internal interfaces and pushing the casts outward toward the interface with Scheme. I now understand more about the overhead that is involved in the encapsulation that I thought was desirable. Rather than an execution overhead, there is a coding overhead. For every type of dynamic cast I may want to use, I need to provide a getter method. And this just covers up a dynamic cast; there's not any reasonable error handling involved in the getter method. That's not very smart, I see now. I wholeheartedly agree with your changes. Thanks for running with this issue. Carl https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
I am convinced by these arguments. Thank you for your patience with me. Hopefully we can get some rats taken care of. Carl https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
On 2018/06/13 03:24:02, dan_faithful.be wrote: On Jun 12, 2018, at 18:18, mailto:carl.d.soren...@gmail.com wrote: > > The tradeoff of having people know about dynamic casting and using it > properly needs to be matched with people not needing to know about > dynamic casting and being able to ignore it. Carl, I appreciate hearing your perspective and I’d like to continue this discussion to the point of reaching a consensus on the design. I perceive that we understand each other’s points and simply disagree. There is nothing new I want to counter with. I will just state that if a contributor were made uncomfortable by dynamic_cast, my two-pronged solution would be (1) gently encourage him to educate himself on this fundamental feature of C++, and (2) over time, rework the software to require fewer casts by preserving more type information in the internal interfaces and pushing the casts outward toward the interface with Scheme. > As I said before, I'm not asking for a reversion. I think I just have a > different tradeoff value model than you have. Clearly, and I’m sure David can contribute a third perspective, and I hope he will choose to. I've been hoping to avoid that. This attempted clean-up and some other work in progress has been about as engaging as pounding sand down a rat hole, The only thing I've found to work yet are cats and poison. Cats scale down the visibility and scale of the problem. Poison at least manages to keep the problem in the house at bay because there you got enough of a rationale to revert to second-generation poisons in-house when the damn critters have literally consumed several kg of first-generation bait (which is reasonably harmless regarding the effects on rat corpse feeders) with diminishing effect. Either require a human bothered about dealing with the problem to a sufficient degree to keep working on it. Such engagement may start with pounding sand down a rat hole. and I have only persevered in it because I considered it a step in the direction of a cleaner system in the long run. If it is valuable, then I would be willing to continue, but if that is not the general perception, The "general perception" is irrelevant as long as the general is not working on the code. However, this work includes reviews. I don't see a future for LilyPond when nobody is working on it, but the future without people bothering to review it is not an inviting prospect, either. It's too big for that to work. Dan's work here is trying to lower the amount of code lookups a typical C++ programmer and LilyPond newbie will have to deal with. It hardcodes a LilyPond code concept as a dynamic cast and little else makes sense (apart from possibly adding an assertion). I can follow his rationale that we'd want to stick in the proper type domain in the first place and want to get there eventually. At the same time, it's a distinction completely irrelevant at the Scheme level where things are dynamically typed. I readily agree that the overall performance impact is small. At the same point of time, you have to declare your types in C++ all the time and overly generic declarations, while individually causing only very little performance drain mostly irrelevant to what the dynamic Scheme type system has to go through all of the time, cause programmers wasting time on figuring out what the code is talking about here. It's bad C++ Feng Shui, distracting from more important work. I'll readily admit the effects on inner architecture for making a house welcoming but am utterly untalented in that regard. Thus I am very much content with those who actually are bothered by details enough to propose changes and argue for them to start clean up. This change here is not actually changing things as much as a bit of sorting allowing for throwing some things out later. So it's a bit pointless to argue for or against it on its own merits and/or make a molehill of the sand to be pounded into a rat hole. I sure hope that we'll get to the rats themselves eventually when we have reduced the number of holes they keep running out of. then I would rather take one step back, restore the code to an acceptable state (probably not quite a straight reversion, though), and never touch it again. If the code were in acceptable state, we'd have more programmers and fewer discussions. I can see that this is necessary at the interface between Scheme and C++, but it doesn’t have to be carried through to all the C++ internals, with each step working to determine the specific type information it requires but neglecting to propagate it to the next step. That loose approach could be continued, but it doesn’t have to be. Should it be continued? I'd prefer to have code enjoyable to work with. What this means is different at the C++ and the Scheme level. If you have to declare your types in a static type lattice, it makes sense declaring them in the manner
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
On Jun 12, 2018, at 18:18, carl.d.soren...@gmail.com wrote: > > The tradeoff of having people know about dynamic casting and using it > properly needs to be matched with people not needing to know about > dynamic casting and being able to ignore it. Carl, I appreciate hearing your perspective and I’d like to continue this discussion to the point of reaching a consensus on the design. I perceive that we understand each other’s points and simply disagree. There is nothing new I want to counter with. I will just state that if a contributor were made uncomfortable by dynamic_cast, my two-pronged solution would be (1) gently encourage him to educate himself on this fundamental feature of C++, and (2) over time, rework the software to require fewer casts by preserving more type information in the internal interfaces and pushing the casts outward toward the interface with Scheme. > As I said before, I'm not asking for a reversion. I think I just have a > different tradeoff value model than you have. Clearly, and I’m sure David can contribute a third perspective, and I hope he will choose to. This attempted clean-up and some other work in progress has been about as engaging as pounding sand down a rat hole, and I have only persevered in it because I considered it a step in the direction of a cleaner system in the long run. If it is valuable, then I would be willing to continue, but if that is not the general perception, then I would rather take one step back, restore the code to an acceptable state (probably not quite a straight reversion, though), and never touch it again. Thence, back to consensus: From what I’ve seen, the existing approach in many (most?) cases is that any kind of grob can be passed to any function and if it’s the right kind of grob, you get what you asked for, and if it’s the wrong kind of grob, you get something else. I can see that this is necessary at the interface between Scheme and C++, but it doesn’t have to be carried through to all the C++ internals, with each step working to determine the specific type information it requires but neglecting to propagate it to the next step. That loose approach could be continued, but it doesn’t have to be. Should it be continued? Regards, — Dan ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
Dan, Thanks for the feedback. I appreciate it. I'm still not convinced that pulling the dynamic casts out of the getter/setter pair is better. You talk about performance penalties of dynamic casting. But your profiling shows that dynamic casting took about 1% of the processing time. Even if you could reduce dynamic casting by 90% (which seems unlikely), you'd only reduce execution time by 0.9%, which seems pretty insignificant. The tradeoff of having people know about dynamic casting and using it properly needs to be matched with people not needing to know about dynamic casting and being able to ignore it. It seems to me that unless there is a significant mistake that is made by code that doesn't know about the dynamic cast, it's better off to hide it. In fact, it seems to me that it would be possible (and maybe preferable) to put necessary error checking once in the setter/getter, rather than having to recreate it multiple times throughout the code base. If dynamic casting were taking up 50% of the process time, or errors in using dynamic casting were responsible for large numbers of bugs, I might feel different. But to me, the benefits I understand from your explanation seem to be not worth the cost of having dynamic casts show up in 21 different files. As I said before, I'm not asking for a reversion. I think I just have a different tradeoff value model than you have. Thanks for your explanation, Carl https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
I remembered another thing about dynamic casting which is not
specifically relevant to the changes in this review, and which should be
pretty well known, but I'll record it here for the sake of completeness.
Wherever there is alternative processing predicated on type, e.g.
if (A *a = dynamic_cast (info.grob ())) { ... }
else if (B *b = dynamic_cast (info.grob ()) { ... }
else if (C *c = dynamic_cast (info.grob ()) { ... }
the reader should stop and consider that perhaps defining a virtual
method would be better. It could be the case that there is only one
block that does something and the others are omitted.
Again, this pattern is more easily recognized with the casts out in the
open than if they were obscured as, info.item(), info.spanner(), etc.
https://codereview.appspot.com/344010043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
To summarize, the principle I advocate is to establish early what kind of object you are dealing with, and once you have paid for that information, retain it as long as you can. In LilyPond it seems I often see the opposite: casts in the lowest depths of the code. Regards, Dan https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
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 (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 (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.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 (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
Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
I realize this issue has already been pushed and closed. But I have a question. What is the harm of having the downcasting in the getter function? An advantage is that we can change the downcasting in one place if it is part of the member functions for the class. If not, we have to change it throughout the code base. Maintainability would seem to be a reason to keep it as it was. I'm not asking for a revert. But I would like to understand the reason it was worth it for Dan to make this change. Mostly for my own understanding of good programming practices. Thanks, Carl https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
