http://codereview.appspot.com/5464045/diff/8002/input/regression/markup-cyclic-reference.ly File input/regression/markup-cyclic-reference.ly (right):
http://codereview.appspot.com/5464045/diff/8002/input/regression/markup-cyclic-reference.ly#newcode2 input/regression/markup-cyclic-reference.ly:2: #(use-modules (scm markup-facility-defs)) Is this required? http://codereview.appspot.com/5464045/diff/8002/ly/toc-init.ly File ly/toc-init.ly (right): http://codereview.appspot.com/5464045/diff/8002/ly/toc-init.ly#newcode4 ly/toc-init.ly:4: %% scheme code now defined in (scm markup-facility-defs) Can you explain why it was necessary to move the stuff in here to markup-facility-defs? It looks like the kind of thing that should continue working. http://codereview.appspot.com/5464045/diff/8002/scm/define-event-classes.scm File scm/define-event-classes.scm (right): http://codereview.appspot.com/5464045/diff/8002/scm/define-event-classes.scm#newcode22 scm/define-event-classes.scm:22: (if (not (defined? 'ly:is-listened-event-class)) (use-modules (lily)) ) Uh, why the conditional here? Isn't it normal to use-modules whenever one needs something defined in a different module, and let use-modules cater for loading the stuff if not already done? http://codereview.appspot.com/5464045/diff/8002/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): http://codereview.appspot.com/5464045/diff/8002/scm/define-markup-commands.scm#newcode370 scm/define-markup-commands.scm:370: x laboris nisi ut aliquip ex ea commodo consequat. What's with the x? http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm File scm/markup-facility-defs.scm (right): http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode34 scm/markup-facility-defs.scm:34: markup* ) markup*? http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode94 scm/markup-facility-defs.scm:94: Use `markup*' in a \\notemode context." Why are you reintroducing markup*? http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode169 scm/markup-facility-defs.scm:169: (ly:debug "Defined ~s function in ~s\n" ,(symbol->string command-name) (current-module)) Is there a reason to insert this debug output including the current module? http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode198 scm/markup-facility-defs.scm:198: (ly:debug "Defined ~s function in ~s\n" ,(symbol->string make-markup-name) (current-module)) Same here. http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode392 scm/markup-facility-defs.scm:392: (defmacro*-public markup* (#:rest body) Again: is there a reason you reintroduce markup*? http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode413 scm/markup-facility-defs.scm:413: ;; (eval-when (load compile eval) ; Guile V2 only Is there a reason you keep this line in after commenting it out? It does not seem to convey useful information. http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode424 scm/markup-facility-defs.scm:424: (ly:message "Current module: ~s" (current-module)) You throw a signal here that I don't see being caught anywhere. http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode458 scm/markup-facility-defs.scm:458: ;;) ; Guile V2 only What is Guile V2 only here? http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode690 scm/markup-facility-defs.scm:690: (set-current-module lilypond-module) Is this line necessary? What happens with it when loading, what when compiling? Makes me nervous. http://codereview.appspot.com/5464045/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
