Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(308)

Issue 344010043: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year ago by Dan Eble
Modified:
1 year ago
Reviewers:
dak, dan, carl.d.sorensen, Carl
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

https://sourceforge.net/p/testlilyissues/issues/5336/ Presenting dynamic casts as simple getters was hiding something that is better left in the open.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+57 lines, -73 lines) Patch
M lily/break-align-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/clef-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/cue-clef-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/dynamic-align-engraver.cc View 2 chunks +16 lines, -10 lines 0 comments Download
M lily/extender-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/grob-array.cc View 1 chunk +1 line, -15 lines 0 comments Download
M lily/grob-info.cc View 2 chunks +1 line, -14 lines 0 comments Download
M lily/hyphen-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/include/grob-array.hh View 1 chunk +0 lines, -2 lines 0 comments Download
M lily/include/grob-info.hh View 1 chunk +0 lines, -2 lines 0 comments Download
M lily/ligature-bracket-engraver.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M lily/ottava-engraver.cc View 1 chunk +8 lines, -6 lines 0 comments Download
M lily/paper-column-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/piano-pedal-align-engraver.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M lily/pointer-group-interface.cc View 2 chunks +2 lines, -1 line 0 comments Download
M lily/pure-from-neighbor-engraver.cc View 2 chunks +7 lines, -3 lines 0 comments Download
M lily/separating-line-group-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/spacing-interface.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/spanner-break-forbid-engraver.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M lily/tab-tie-follow-engraver.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M lily/volta-engraver.cc View 1 chunk +3 lines, -2 lines 0 comments Download

Messages

Total messages: 9
Carl
I realize this issue has already been pushed and closed. But I have a question. ...
1 year ago (2018-06-11 22:45:12 UTC) #1
Dan Eble
My intent was to make it clear to the person using these objects what is ...
1 year ago (2018-06-12 00:38:35 UTC) #2
Dan Eble
To summarize, the principle I advocate is to establish early what kind of object you ...
1 year ago (2018-06-12 00:59:42 UTC) #3
Dan Eble
I remembered another thing about dynamic casting which is not specifically relevant to the changes ...
1 year ago (2018-06-12 21:37:55 UTC) #4
Carl
Dan, Thanks for the feedback. I appreciate it. I'm still not convinced that pulling the ...
1 year ago (2018-06-12 22:18:41 UTC) #5
dan_faithful.be
On Jun 12, 2018, at 18:18, carl.d.sorensen@gmail.com wrote: > > The tradeoff of having people ...
1 year ago (2018-06-13 03:24:02 UTC) #6
dak
On 2018/06/13 03:24:02, dan_faithful.be wrote: > On Jun 12, 2018, at 18:18, mailto:carl.d.sorensen@gmail.com wrote: > ...
1 year ago (2018-06-13 08:12:44 UTC) #7
Carl
I am convinced by these arguments. Thank you for your patience with me. Hopefully we ...
1 year ago (2018-06-13 16:04:53 UTC) #8
Carl
1 year ago (2018-06-13 16:15:07 UTC) #9
Message was sent while issue was closed.
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b