Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(516)

Issue 4822055: Proper loglevels: cmd-line option --loglevel=NONE/ERROR/WARN/PROGRESS/INFO/DEBUG (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 8 months ago by Reinhold
Modified:
2 years, 8 months ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Proper loglevels: cmd-line option --loglevel=NONE/ERROR/WARN/PROGRESS/INFO/DEBUG

Allow the user to specify which messages (s)he wants to see on stderr:
The lilypond code basically stays the same, I only added a loglevel
global variable, which is used in the warning/error*/progress*/success
functions (ly:warning, ly:error, etc. in Scheme). If the proper level
is not set for a message, it is not printed.

There are only two larger changes:
-) Global var be_verbose_global replaced by loglevel (much finer-grained)
-) New functions (full)debug_output, which replaces code like:
   if (be_verbose_global) {
	   progress_indication (...)
	 }

All changes were done to warn.cc and to the member of the Input class
(apparently, we have two parallel error-reporting "frameworks" in
lilypond...).

Note for all functions in warn.cc (not for the members of Input):
The debug_output function (and progress_indication and message) have
an optional argument to specify whether the output of the message
should always start on a new line or continue the previous output.

Documentation for this new feature is still missing (both in the AU
as well as in the CG).

Patch Set 1 #

Total comments: 28

Patch Set 2 : Move loglevel logic to warn.cc/hh, provide accessors/setters; Add BASIC_PROGRESS, remove FULLDEBUG #

Patch Set 3 : Include optional message location, move scheme functions to warn-scheme, clean up code, etc. #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+492 lines, -280 lines) Patch
M flower/include/warn.hh View 1 2 1 chunk +38 lines, -7 lines 1 comment Download
M flower/warn.cc View 1 2 1 chunk +125 lines, -25 lines 3 comments Download
A input/regression/loglevels.ly View 1 1 chunk +41 lines, -0 lines 0 comments Download
M lily/all-font-metrics.cc View 1 3 chunks +4 lines, -8 lines 0 comments Download
M lily/font-config.cc View 1 2 chunks +5 lines, -8 lines 0 comments Download
M lily/font-config-scheme.cc View 1 2 chunks +4 lines, -4 lines 0 comments Download
M lily/general-scheme.cc View 1 2 3 chunks +7 lines, -84 lines 0 comments Download
M lily/global-context-scheme.cc View 1 1 chunk +1 line, -2 lines 0 comments Download
M lily/guile-init.cc View 1 1 chunk +3 lines, -3 lines 0 comments Download
M lily/includable-lexer.cc View 1 2 3 chunks +5 lines, -16 lines 0 comments Download
M lily/include/input.hh View 1 2 2 chunks +5 lines, -2 lines 0 comments Download
M lily/include/main.hh View 1 1 chunk +0 lines, -1 line 0 comments Download
M lily/input.cc View 1 2 1 chunk +29 lines, -15 lines 1 comment Download
M lily/lexer.ll View 1 chunk +1 line, -2 lines 0 comments Download
M lily/lily-guile.cc View 1 1 chunk +2 lines, -4 lines 0 comments Download
M lily/main.cc View 1 2 6 chunks +20 lines, -9 lines 1 comment Download
M lily/paper-score.cc View 1 1 chunk +3 lines, -4 lines 0 comments Download
M lily/performance.cc View 1 1 chunk +3 lines, -6 lines 0 comments Download
M lily/pfb-scheme.cc View 1 4 chunks +4 lines, -8 lines 0 comments Download
M lily/program-option-scheme.cc View 1 1 chunk +10 lines, -3 lines 0 comments Download
M lily/relocate.cc View 1 9 chunks +18 lines, -25 lines 0 comments Download
M lily/system.cc View 1 3 chunks +4 lines, -8 lines 0 comments Download
M lily/ttf.cc View 1 4 chunks +4 lines, -8 lines 0 comments Download
A lily/warn-scheme.cc View 1 2 1 chunk +139 lines, -0 lines 0 comments Download
M scm/backend-library.scm View 1 chunk +1 line, -4 lines 0 comments Download
M scm/define-note-names.scm View 1 chunk +1 line, -2 lines 0 comments Download
M scm/lily.scm View 1 2 3 chunks +9 lines, -11 lines 0 comments Download
M scm/lily-library.scm View 1 2 1 chunk +6 lines, -11 lines 0 comments Download

Messages

Total messages: 17
Reinhold
http://codereview.appspot.com/4822055/diff/1/lily/main.cc File lily/main.cc (right): http://codereview.appspot.com/4822055/diff/1/lily/main.cc#newcode170 lily/main.cc:170: {0, "silent", 's', _i ("no progress, only error messages ...
2 years, 8 months ago #1
MikeSol
LGTM. Cheers, MS
2 years, 8 months ago #2
Reinhold
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#newcode87 lily/input.cc:87: ::print_message (s); Can leave out the :: here http://codereview.appspot.com/4822055/diff/1/lily/main.cc ...
2 years, 8 months ago #3
hanwenn
overall commment: the parts that do the reporting should not need to know about the ...
2 years, 8 months ago #4
Ian Hulin (gmail)
FWIW LGTM apart from attached comments, I'm currently working on main.cc and lily.scm as part ...
2 years, 8 months ago #5
Reinhold
On 2011/07/30 17:10:00, Ian Hulin (gmail) wrote: > I'm currently working on main.cc and lily.scm ...
2 years, 8 months ago #6
Reinhold
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 ...
2 years, 8 months ago #7
Reinhold
2 years, 8 months ago #8
Reinhold
2 years, 8 months ago #9
reinhold_kainhofer.com
Am Samstag, 30. Juli 2011, 18:28:32 schrieben Sie: > overall commment: the parts that do ...
2 years, 8 months ago #10
Reinhold
2 years, 8 months ago #11
Carl
I really like this -- thanks for taking it on. Two small comments. http://codereview.appspot.com/4822055/diff/1/lily/font-config-scheme.cc File ...
2 years, 8 months ago #12
J_lowe
Passes make and reg test.
2 years, 8 months ago #13
Graham Percival (old account)
awesome work, I really love this. Unfortunately it cannot apply directly to git master due ...
2 years, 8 months ago #14
hanwenn
On Sat, Jul 30, 2011 at 2:42 PM, Reinhold Kainhofer <reinhold@kainhofer.com> wrote: > Am Samstag, ...
2 years, 8 months ago #15
Reinhold
http://codereview.appspot.com/4822055/diff/18001/scripts/build/mf2pt1.pl File scripts/build/mf2pt1.pl (right): http://codereview.appspot.com/4822055/diff/18001/scripts/build/mf2pt1.pl#newcode1 scripts/build/mf2pt1.pl:1: #!@PERL@ Apparently, I dit a git fetch, but didn't ...
2 years, 8 months ago #16
Keith
2 years, 8 months ago #17
I like it.

http://codereview.appspot.com/4822055/diff/6003/flower/include/warn.hh
File flower/include/warn.hh (right):

http://codereview.appspot.com/4822055/diff/6003/flower/include/warn.hh#newcode41
flower/include/warn.hh:41: // There is no separation between progress and other
info messages:
but define LOGLEVEL_INFO with an | LOG_INFO anyway, 
just in case somebody tries to send a message with LOG_INFO

http://codereview.appspot.com/4822055/diff/6003/flower/warn.cc
File flower/warn.cc (right):

http://codereview.appspot.com/4822055/diff/6003/flower/warn.cc#newcode54
flower/warn.cc:54: debug_output (_f ("Log level set to `%d'\n", loglevel));
Quotes around the number, `271', confused me

http://codereview.appspot.com/4822055/diff/6003/flower/warn.cc#newcode132
flower/warn.cc:132: /* Display a fatal error message.  Also exits lilypond.  */
Can you now remove the function non_fatal_error() in favor of error() ?  It
looks strange to use non_fatal_error to dipaly a fatal error message.

http://codereview.appspot.com/4822055/diff/6003/flower/warn.cc#newcode180
flower/warn.cc:180: // Use the progress loglevel for all normal messages
(including progress msg)
Confusing. Input::message() filters with LOG_INFO; shouldn't message() do the
same?  (at least until the duplicate code is eventually merged)

http://codereview.appspot.com/4822055/diff/6003/lily/input.cc
File lily/input.cc (right):

http://codereview.appspot.com/4822055/diff/6003/lily/input.cc#newcode130
lily/input.cc:130: print_message (LOG_INFO, s);
has no effect, unless LOG_INFO is included in one of the bitmasks.

http://codereview.appspot.com/4822055/diff/6003/lily/main.cc
File lily/main.cc (right):

http://codereview.appspot.com/4822055/diff/6003/lily/main.cc#newcode172
lily/main.cc:172: "NONE, ERROR, WARNING, PROGRESS (default), DEBUG and
FULLDEBUG.")
Most of the new lines are too long for an 80-character terminal.
You wanted to remove FULLDEBUG.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1278:e6ce13d99bf5