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

Issue 14460043: Move New_dynamic_engraver over the unused Dynamic_engraver (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 5 months ago by dak
Modified:
10 years, 1 month ago
Reviewers:
thomasmorley651
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Move New_dynamic_engraver over the unused Dynamic_engraver Consists of commits: Rename New_dynamic_engraver to Dynamic_engraver in lily/dynamic-engraver.cc Rename lily/new-dynamic-engraver.cc to lily/dynamic-engraver.cc Run scripts/auxiliar/update-with-convert-ly.sh convert-ly rule in preparation for renaming New_dynamic_engraver to Dynamic_engraver Remove Dynamic_engraver. Run scripts/auxiliar/makelsr.py Snippet engravers-one-by-one should use New_dynamic_engraver

Patch Set 1 #

Patch Set 2 : Do more of the replacement using convert-ly #

Unified diffs Side-by-side diffs Delta from patch set Stats (+211 lines, -661 lines) Patch
M Documentation/cs/learning/fundamental.itely View 2 chunks +2 lines, -3 lines 0 comments Download
M Documentation/de/learning/fundamental.itely View 2 chunks +2 lines, -3 lines 0 comments Download
M Documentation/es/learning/fundamental.itely View 2 chunks +2 lines, -3 lines 0 comments Download
M Documentation/fr/learning/fundamental.itely View 2 chunks +2 lines, -2 lines 0 comments Download
M Documentation/hu/learning/fundamental.itely View 2 chunks +2 lines, -3 lines 0 comments Download
M Documentation/it/learning/fundamental.itely View 2 chunks +2 lines, -3 lines 0 comments Download
M Documentation/ja/learning/fundamental.itely View 2 chunks +2 lines, -3 lines 0 comments Download
M Documentation/learning/fundamental.itely View 2 chunks +2 lines, -3 lines 0 comments Download
M Documentation/nl/learning/fundamental.itely View 2 chunks +2 lines, -3 lines 0 comments Download
M Documentation/snippets/engravers-one-by-one.ly View 1 2 chunks +2 lines, -1 line 0 comments Download
M lily/dynamic-engraver.cc View 3 chunks +177 lines, -325 lines 0 comments Download
D lily/new-dynamic-engraver.cc View 1 chunk +0 lines, -305 lines 0 comments Download
M ly/engraver-init.ly View 3 chunks +3 lines, -4 lines 0 comments Download
M python/convertrules.py View 1 1 chunk +11 lines, -0 lines 0 comments Download

Messages

Total messages: 5
dak
Do more of the replacement using convert-ly
10 years, 5 months ago (2013-10-06 14:38:49 UTC) #1
thomasmorley651
Although, I can't review C++, I've applied the patch for testing (hopefully without mistake) Testing ...
10 years, 5 months ago (2013-10-06 23:00:25 UTC) #2
dak
On 2013/10/06 23:00:25, thomasmorley651 wrote: > Although, I can't review C++, I've applied the patch ...
10 years, 5 months ago (2013-10-06 23:10:16 UTC) #3
thomasmorley651
On 2013/10/06 23:10:16, dak wrote: > On 2013/10/06 23:00:25, thomasmorley651 wrote: [...] > > Or ...
10 years, 5 months ago (2013-10-06 23:29:23 UTC) #4
dak
10 years, 5 months ago (2013-10-06 23:38:58 UTC) #5
On 2013/10/06 23:29:23, thomasmorley651 wrote:
> On 2013/10/06 23:10:16, dak wrote:
> > On 2013/10/06 23:00:25, thomasmorley651 wrote:
> [...]
> > > Or am I completely wrong and this patch has nothing to do with the problem
> > > above?
> > 
> > In this case, you are completely wrong.  
> [...]
> Anyway, thanks for clarifying.

I have no idea how buggy the current code is, and it may well be that the old
engraver dealt better with this particular situation.  But I would be surprised
if its C++ code has not gathered bit rot by now after being out of use for so
long, and there was a reason for the new engraver(s) so ditching them again
presumably is not an alternative.

The patch just renames things (partly with the help of convert-ly) but uses
exactly the same code paths unless you used Dynamic_engraver in a file of your
own.
Sign in to reply to this message.

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