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/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel