lilypond-devel
[Top][All Lists]
Advanced

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

Re: Proper loglevels: cmd-line option --loglevel=NONE/ERROR/WARN/PROGRES


From: ianhulin44
Subject: Re: Proper loglevels: cmd-line option --loglevel=NONE/ERROR/WARN/PROGRESS/INFO/DEBUG (issue4822055)
Date: Sat, 30 Jul 2011 17:10:00 +0000

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/



reply via email to

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