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

Issue 344050043: Issue 5362: Generalize context searches

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 2 weeks ago by Dan Eble
Modified:
3 months, 2 weeks ago
Reviewers:
dak, dan
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

https://sourceforge.net/p/testlilyissues/issues/5362/ Combine code for \new, \context, and various internal searches. This includes a small block of disabled code which will fix input/regression/context-find-parent.ly once it is enabled. I want the reviewers to see it, but I don't want to mix the behavioral change into this major reorganization. Separate the following from the act of finding a context: 1. forbid access to the Global context 2. prevent creation of multiple Scores under the Global context 3. warn if the desired context is not found or created I've tried to judge where to do or not to do these three things based on the previous implementation. Let me know if you think I've judged poorly or where the previous implementation was not worth following. In a second commit: Rename the music property descend-only to search-direction; change its type from a boolean to a direction.

Patch Set 1 #

Total comments: 7

Patch Set 2 : fix typo in comment #

Total comments: 3

Patch Set 3 : ditch templates #

Unified diffs Side-by-side diffs Delta from patch set Stats (+312 lines, -178 lines) Patch
M input/regression/context-defaultchild-cycle.ly View 1 chunk +2 lines, -2 lines 0 comments Download
M lily/change-iterator.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M lily/context.cc View 1 2 5 chunks +157 lines, -113 lines 0 comments Download
M lily/context-scheme.cc View 1 chunk +2 lines, -9 lines 0 comments Download
M lily/context-specced-music-iterator.cc View 1 chunk +15 lines, -9 lines 0 comments Download
M lily/global-context.cc View 1 chunk +24 lines, -0 lines 0 comments Download
M lily/include/context.hh View 1 2 5 chunks +76 lines, -25 lines 0 comments Download
M lily/include/global-context.hh View 1 chunk +5 lines, -0 lines 0 comments Download
M lily/quote-iterator.cc View 1 chunk +9 lines, -3 lines 0 comments Download
M lily/simultaneous-music-iterator.cc View 1 chunk +17 lines, -11 lines 0 comments Download
M scm/define-music-properties.scm View 2 chunks +1 line, -2 lines 0 comments Download
M scm/music-functions.scm View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 15
Dan Eble
fix typo in comment
3 months, 2 weeks ago (2018-06-29 01:19:20 UTC) #1
dak
In summary: at the current point of time the added complexity because of the employed ...
3 months, 2 weeks ago (2018-07-01 13:21:45 UTC) #2
dak
On 2018/07/01 13:21:45, dak wrote: > NLGTM Man, I use this stuff too rarely. Maybe ...
3 months, 2 weeks ago (2018-07-01 13:25:35 UTC) #3
Dan Eble
(initial feedback—more later) https://codereview.appspot.com/344050043/diff/1/lily/global-context.cc File lily/global-context.cc (right): https://codereview.appspot.com/344050043/diff/1/lily/global-context.cc#newcode143 lily/global-context.cc:143: return score; On 2018/07/01 13:21:42, dak ...
3 months, 2 weeks ago (2018-07-01 14:09:28 UTC) #4
Dan Eble
https://codereview.appspot.com/344050043/diff/20001/lily/change-iterator.cc File lily/change-iterator.cc (right): https://codereview.appspot.com/344050043/diff/20001/lily/change-iterator.cc#newcode67 lily/change-iterator.cc:67: Context::warning_cannot_find (origin, to_type, to_id); On 2018/07/01 13:21:43, dak wrote: ...
3 months, 2 weeks ago (2018-07-01 16:48:09 UTC) #5
dak
On 2018/07/01 16:48:09, Dan Eble wrote: > https://codereview.appspot.com/344050043/diff/20001/lily/change-iterator.cc > File lily/change-iterator.cc (right): > > https://codereview.appspot.com/344050043/diff/20001/lily/change-iterator.cc#newcode67 ...
3 months, 2 weeks ago (2018-07-01 17:14:25 UTC) #6
dan_faithful.be
> On Jul 1, 2018, at 13:14, dak@gnu.org wrote: > > Something like > origin->warning ...
3 months, 2 weeks ago (2018-07-01 18:17:16 UTC) #7
dak
On 2018/07/01 18:17:16, dan_faithful.be wrote: > > On Jul 1, 2018, at 13:14, mailto:dak@gnu.org wrote: ...
3 months, 2 weeks ago (2018-07-01 19:08:23 UTC) #8
Dan Eble
ditch templates
3 months, 2 weeks ago (2018-07-01 19:36:50 UTC) #9
dan_faithful.be
On Jul 1, 2018, at 15:08, dak@gnu.org wrote: > > We currently have > > ...
3 months, 2 weeks ago (2018-07-01 19:51:45 UTC) #10
dak
On 2018/07/01 14:09:28, Dan Eble wrote: > (initial feedback—more later) > > https://codereview.appspot.com/344050043/diff/1/lily/global-context.cc > File ...
3 months, 2 weeks ago (2018-07-01 20:40:26 UTC) #11
dak
Dan Eble <dan@faithful.be> writes: > On Jul 1, 2018, at 15:08, dak@gnu.org wrote: >> >> ...
3 months, 2 weeks ago (2018-07-01 20:44:00 UTC) #12
dan_faithful.be
On Jul 1, 2018, at 16:40, dak@gnu.org wrote: > > Premature optimization is the root ...
3 months, 2 weeks ago (2018-07-02 00:16:31 UTC) #13
dak
On 2018/07/02 00:16:31, dan_faithful.be wrote: > On Jul 1, 2018, at 16:40, mailto:dak@gnu.org wrote: > ...
3 months, 2 weeks ago (2018-07-02 07:59:30 UTC) #14
Dan Eble
3 months, 2 weeks ago (2018-07-02 23:30:56 UTC) #15
message: On 2018/07/01 18:17:16, dan_faithful.be wrote:
> > On Jul 1, 2018, at 13:14, mailto:dak@gnu.org wrote:
> > 
> > Something like
> > origin->warning (_f ("Cannot find or create context: %s",
> > ctx->identification ()))
...
> Probably worth doing separately for its own sake.

https://codereview.appspot.com/349740043/
Sign in to reply to this message.

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