FWIW LGTM apart from attached comments, I'm currently working on main.cc and lily.scm as part of T1686, and may need help with merging if this patch gets pushed first.
Is your current design extensible to allow us to segment the old verbose option by function as well as debug trace level? Maybe have a --tracelevel option with values like INITIALIZE, PATH, FONTS, SCHEME_DEBUG which would OR bits in the upper byte of loglevel. That way we could have even finer control of the --loglevel=DEBUG output. http://codereview.appspot.com/4822055/diff/1/flower/warn.cc File flower/warn.cc (right): http://codereview.appspot.com/4822055/diff/1/flower/warn.cc#newcode86 flower/warn.cc:86: exit (1); Tell the user you're crashing with a fatal error: error (string (s); { if (loglevel & LOG_ERROR) print_message (_f ("fatal error: %s", s.c_str()) + "\n"); exit (1); } http://codereview.appspot.com/4822055/diff/1/lily/all-font-metrics.cc File lily/all-font-metrics.cc (right): http://codereview.appspot.com/4822055/diff/1/lily/all-font-metrics.cc#newcode91 lily/all-font-metrics.cc:91: debug_output ("[" + string (pango_fn), true); // start on a new line Why do the logging output in two calls? Why not debug_output ("[" + string (pango_fn)+ "]", true); // start on a new line here and lose line 102? Ian http://codereview.appspot.com/4822055/diff/1/lily/all-font-metrics.cc#newcode102 lily/all-font-metrics.cc:102: debug_output ("]", false); Add the "]" to the previous debug_output call and get rid of this line. (See previous) http://codereview.appspot.com/4822055/diff/1/lily/all-font-metrics.cc#newcode128 lily/all-font-metrics.cc:128: debug_output ("[" + file_name, true); // start on a new line As for line 91 - do all the output in this call and get rid of the other debug_output call below http://codereview.appspot.com/4822055/diff/1/lily/font-config.cc File lily/font-config.cc (right): http://codereview.appspot.com/4822055/diff/1/lily/font-config.cc#newcode56 lily/font-config.cc:56: debug_output (_f ("adding font directory: %s", dir.c_str ())); Capitalize "Adding" - for consistency with other messages, which start "Initializing..." and "Building..." http://codereview.appspot.com/4822055/diff/1/lily/input.cc File lily/input.cc (right): http://codereview.appspot.com/4822055/diff/1/lily/input.cc#newcode94 lily/input.cc:94: print_message (_f ("error: %s", s)); print_message ( _f ("fatal error: %s", s)); Why do you pass s here and in the similar place in warn.cc you need to pass s.c_str()? http://codereview.appspot.com/4822055/diff/1/lily/program-option-scheme.cc File lily/program-option-scheme.cc (right): http://codereview.appspot.com/4822055/diff/1/lily/program-option-scheme.cc#newcode259 lily/program-option-scheme.cc:259: "Was berbose output requested, i.e. loglevel at least @code{DEBUG}?") On 2011/07/29 19:51:00, Reinhold wrote:
should be "verbose"
Or maybe "Was full debug output requested, i.e. loglevel at least @code(DEBUG)?" http://codereview.appspot.com/4822055/diff/1/lily/program-option-scheme.cc#newcode271 lily/program-option-scheme.cc:271: Nice feature. If you're allowing this to be accessed from Scheme, how about translating the number back to the keyword value as input on the command line (NONE, PROGRESS, ...) http://codereview.appspot.com/4822055/diff/1/scm/lily.scm File scm/lily.scm (right): http://codereview.appspot.com/4822055/diff/1/scm/lily.scm#newcode288 scm/lily.scm:288: ;; a newline in this case Needs to use guile %load-hook callback, <<<<<< (if (ly:loglevel <> 0) (set! %load-hook (lambda (filename) (ly:debug ( _ "\t[Loading ~a] ...\n") filename)))) (define-public (ly:load x) (if (not file-name) (ly:error (_ "cannot find: ~A") x)) (primitive-load-path file-name)) ly:load is getting a re-write in any case as part of the fix for T1686 http://codereview.appspot.com/4822055/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
