Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by address@hidden)

[dak]

On 2018/06/13 03:24:02, dan_faithful.be wrote:
> On Jun 12, 2018, at 18:18, mailto:address@hidden 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 conveying their meaning.  So I am fine with your longer
perspective.  At the same time, this review is about code changes in
preparation of making the longer perspective a more appealing and
defined task to work on and so discussing its respective merits is
somewhat pointless and wasting time and energy we'd rather want spent on
actual improvements.

It's good that we have comfortable review structures in place suitable
for dealing with large-scale changes.  I am just afraid that current
development speed tends to makke for unproportional response.  Sort of
an overblown autoimmune response, a bit self-defeating.

Summary: I'd want Dan to go ahead.  Not as much based on the individual
goals achieved with this particular patch but his overall goal of making
the interior design of LilyPond more hospitable to visiting C++
programmers.  It's not just the code but also the culture that wants
adjustment in that respect.  This patch and review engaged more energy
than called for and that's a thing that wants fixing as badly as the
code.

https://codereview.appspot.com/344010043/