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

Issue 341150043: Issue 5310: find_top_context () maintenance

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

Description

https://sourceforge.net/p/testlilyissues/issues/5310/ * Let find_top_context () deal with references instead of pointers because every non-empty tree has a top. * Call find_top_context () rather than get_global_context () where the caller uses the result as a mere Context.

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -18 lines) Patch
M lily/context.cc View 1 chunk +6 lines, -6 lines 3 comments Download
M lily/include/context.hh View 1 chunk +1 line, -1 line 0 comments Download
M lily/lyric-combine-music-iterator.cc View 3 chunks +8 lines, -5 lines 0 comments Download
M lily/score-engraver.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M lily/score-performer.cc View 3 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 13
Dan Eble
1 year, 7 months ago (2018-04-21 20:39:49 UTC) #1
dak
https://codereview.appspot.com/341150043/diff/1/lily/context.cc File lily/context.cc (right): https://codereview.appspot.com/341150043/diff/1/lily/context.cc#newcode723 lily/context.cc:723: find_top_context (Context &where) What problem are you trying to ...
1 year, 7 months ago (2018-04-21 20:58:32 UTC) #2
Carl
It seems like if there is a problem that this solves, there should be a ...
1 year, 7 months ago (2018-04-22 01:18:22 UTC) #3
Dan Eble
On 2018/04/22 01:18:22, Carl wrote: > It seems like if there is a problem that ...
1 year, 7 months ago (2018-04-22 13:19:37 UTC) #4
Dan Eble
https://codereview.appspot.com/341150043/diff/1/lily/context.cc File lily/context.cc (right): https://codereview.appspot.com/341150043/diff/1/lily/context.cc#newcode723 lily/context.cc:723: find_top_context (Context &where) On 2018/04/21 20:58:32, dak wrote: > ...
1 year, 7 months ago (2018-04-22 13:19:46 UTC) #5
Dan Eble
On 2018/04/22 13:19:46, Dan Eble wrote: > of calls in some scope: if (Context *c ...
1 year, 7 months ago (2018-04-22 13:21:46 UTC) #6
dak
https://codereview.appspot.com/341150043/diff/1/lily/context.cc File lily/context.cc (right): https://codereview.appspot.com/341150043/diff/1/lily/context.cc#newcode723 lily/context.cc:723: find_top_context (Context &where) On 2018/04/22 13:19:45, Dan Eble wrote: ...
1 year, 7 months ago (2018-04-22 13:37:50 UTC) #7
Dan Eble
On 2018/04/22 13:37:50, dak wrote: > So again I don't see what problem you are ...
1 year, 7 months ago (2018-04-22 13:56:01 UTC) #8
dak
On 2018/04/22 13:56:01, Dan Eble wrote: > On 2018/04/22 13:37:50, dak wrote: > > So ...
1 year, 7 months ago (2018-04-22 14:26:45 UTC) #9
Dan Eble
David, would you accept a template class wrapping a non-null pointer, perhaps non_null_ptr<T>? Conversion to ...
1 year, 7 months ago (2018-04-23 23:55:44 UTC) #10
dak
On 2018/04/23 23:55:44, Dan Eble wrote: > David, would you accept a template class wrapping ...
1 year, 6 months ago (2018-04-25 10:44:22 UTC) #11
Dan Eble
On 2018/04/25 10:44:22, dak wrote: > Please note that all of your proposals so far ...
1 year, 6 months ago (2018-04-25 23:05:08 UTC) #12
dak
1 year, 6 months ago (2018-04-29 21:23:18 UTC) #13
On 2018/04/25 23:05:08, Dan Eble wrote:
> On 2018/04/25 10:44:22, dak wrote:
> > It still has the disadvantage of adding a new class as baggage, making it
harder
> > for newcomers interpreting code.  In particular the necessity of overloading
*,
> > &, and -> operators raises the amount of knowledge required for maintenance
and
> > debugging.  I am rather doubtful that this would be a good tradeoff so I
would
> > not want to encourage you to indulge in a significant amount of work here
which
> > I may not want to see added to the code base after all.
> 
> If you're at least open to reviewing something concrete, I'll put something
> together, but not today.  Thanks for your time so far.

I have forgotten one consideration: we convert pointers along the class
hierarchy (like between Grob * and Item *) and a similar conversion of a
user-created type would not likely work in a comparable manner.
Sign in to reply to this message.

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