lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Create a two-argument form of define-event-class (issue 10965043)


From: thomasmorley65
Subject: Create a two-argument form of define-event-class (issue 10965043)
Date: Fri, 05 Jul 2013 23:20:48 +0000

I didn't apply the patch and I can't review C++, though, after a first
quick glance, some nitpicks:


https://codereview.appspot.com/10965043/diff/1/scm/define-event-classes.scm
File scm/define-event-classes.scm (right):

https://codereview.appspot.com/10965043/diff/1/scm/define-event-classes.scm#newcode87
scm/define-event-classes.scm:87: (define ancestor-lookup
(make-hash-table (length all-event-classes)))
I'm not convinced about the naming.

'ancestor' is defined in titling-init.ly, ofcourse with different
functionality.
'ancestor-lookup' is a _very_ generic naming, and it's usage _is_
generic in a certain way.
Though, I'd prefer something like 'ancestor-all-event-classes-lookup'.
No, that's to long.
But you get the point.

https://codereview.appspot.com/10965043/diff/1/scm/define-grobs.scm
File scm/define-grobs.scm (right):

https://codereview.appspot.com/10965043/diff/1/scm/define-grobs.scm#newcode28
scm/define-grobs.scm:28: (define-session-public all-grob-descriptions
Very nice!

https://codereview.appspot.com/10965043/diff/1/scm/document-music.scm
File scm/document-music.scm (right):

https://codereview.appspot.com/10965043/diff/1/scm/document-music.scm#newcode33
scm/document-music.scm:33: (let* ((class (ly:camel-case->lisp-identifier
(car entry)))
Apart from using tabs I'm not able to see any difference to the old
file.

No offense, just curious about it:
I was told not to use tabs, what's the reason for using them here?

https://codereview.appspot.com/10965043/diff/1/scm/document-music.scm#newcode89
scm/document-music.scm:89: (props (cdr obj))
Same here

https://codereview.appspot.com/10965043/



reply via email to

[Prev in Thread] Current Thread [Next in Thread]