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

Issue 561290043: Issue 5309, take 2: find_global_context () and find_score_context () (Closed)

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

Description

1: find_score_context () Translator::find_score_context () finds the Score context enclosing the Translator in a way similar to (but slightly more direct than) \set Score.whatever = ... rather than depending on the global context. Global_context::get_score_context () returns the first child, as before, but it is no longer a virtual method of Context, and it is used in fewer places. 2: find_global_context () Replace c->get_global_context () with find_global_context (c). This function does not need to access any private information of Context, so pulling it out simplifies the interface of Context. Calling it "find" rather than "get" is supposed to imply more work (walking the tree). Implement find_global_context (c) as find_top_context (c) plus a type check. This increases code reuse and requires fewer dynamic casts. Abort the program if the top context is not a Global context, otherwise most callers would dereference null pointers. * * * These changes update and expand on changes previously reviewed in http://codereview.appspot.com/346750043.

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -81 lines) Patch
M lily/bar-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/completion-note-heads-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/completion-rest-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/context.cc View 5 chunks +8 lines, -33 lines 2 comments Download
M lily/context-property.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M lily/double-percent-repeat-engraver.cc View 2 chunks +2 lines, -3 lines 0 comments Download
M lily/engraver-group.cc View 2 chunks +14 lines, -14 lines 0 comments Download
M lily/forbid-break-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/global-context.cc View 1 chunk +11 lines, -0 lines 0 comments Download
M lily/include/context.hh View 1 chunk +0 lines, -2 lines 0 comments Download
M lily/include/global-context.hh View 2 chunks +6 lines, -1 line 0 comments Download
M lily/include/translator.hh View 2 chunks +4 lines, -3 lines 0 comments Download
M lily/ligature-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/measure-grouping-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/paper-column-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/partial-iterator.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M lily/percent-repeat-engraver.cc View 1 chunk +1 line, -1 line 0 comments Download
M lily/spanner-break-forbid-engraver.cc View 1 chunk +1 line, -3 lines 0 comments Download
M lily/timing-translator.cc View 3 chunks +2 lines, -4 lines 0 comments Download
M lily/translator.cc View 1 chunk +8 lines, -4 lines 0 comments Download

Messages

Total messages: 3
lemzwerg
LGTM, thanks! https://codereview.appspot.com/561290043/diff/583280043/lily/context.cc File lily/context.cc (right): https://codereview.appspot.com/561290043/diff/583280043/lily/context.cc#newcode116 lily/context.cc:116: if (Context *score = gthis->get_score_context ()) Not ...
3 months ago (2020-01-03 16:18:10 UTC) #1
Dan Eble
On 2020/01/03 16:18:10, lemzwerg wrote: > lily/context.cc:116: if (Context *score = gthis->get_score_context ()) > Not ...
3 months ago (2020-01-03 16:34:14 UTC) #2
dak
3 months ago (2020-01-03 21:21:33 UTC) #3
https://codereview.appspot.com/561290043/diff/583280043/lily/context.cc
File lily/context.cc (right):

https://codereview.appspot.com/561290043/diff/583280043/lily/context.cc#newco...
lily/context.cc:116: if (Context *score = gthis->get_score_context ())
On 2020/01/03 16:18:09, lemzwerg wrote:
> Not sure whether compilers warn about the `if (... = ...)` construct.  Perhaps
> the `Context *` prevents it.  Otherwise I suggest another level of
parentheses.

They don't.  lily/parser.yy is full of it (and a number of other files as well)
and I have no idea where you want to place "another level of parentheses".  It
certainly is not allowed around the declaration.
Sign in to reply to this message.

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