Get rid of some compiler warnings

Get rid of some compiler warnings

-) Some "unused parameter" warnings
-) Some "ignoring return value" are a bit trickier. They are bad coding style:
    o) the file-name.cc one returns a string with undefined contents if getcwd
       fails, which might either crash guile or lead to other bugs.
    o) the lily-parser-scheme.cc warning was also a programming error (if you
       used --output=dir/file, and dir/ had the wrong permissions, then
       lilypond would not print an error, but simply put the output file into
       the current directory without telling the user!).
			 In this case (output dir explicitly requested, but not possible
			 to change there), I think it's best to exit lilypond with an
			 error.
-) signed/unsigned warning in glissando-engraver.cc, where we already
   had a check for negative values, so explicitly casting to unsigned
	 shuts up the compiler.
-) Wrap #(set-paper-size "a6") in the regtests with a \paper block
-) fix bad texinfo code in function documentation

[Carl, 2011-08-09 21:20:29]

LGTM, with a few comments.

Thanks,

Carl

http://codereview.appspot.com/4854049/diff/2001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/4854049/diff/2001/lily/skyline.cc#newcode417
lily/skyline.cc:417: buildings_ = internal_build_skyline (&boxes,
horizon_padding, a, UP);
What if instead of changing X_AXIS to a in line 417, we checked a to see if it
were equal to X_AXIS and issued a warning (or maybe a programming error)?

Note that we have UP hardcoded as well, so at this point I don't think we ought
to remove the hardcoding on only the a.

http://codereview.appspot.com/4854049/diff/2001/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/4854049/diff/2001/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:894: (_i "Scale @var{arg} up by a factor of
2^@var{dur}*(2-(1/2)^@var{dots}).")
I think the whole expression 2^... should be wrapped in a @code{} block.

Also, should this be part of the warning fix patch?

http://codereview.appspot.com/4854049/diff/5001/lily/lyric-combine-music-iter...
File lily/lyric-combine-music-iterator.cc (right):

http://codereview.appspot.com/4854049/diff/5001/lily/lyric-combine-music-iter...
lily/lyric-combine-music-iterator.cc:224:
Lyric_combine_music_iterator::check_new_context (SCM /*sev*/)
Why not /*SCM sev*/ with a FIXME?

[Keith, 2011-08-10 04:39:24]

Reinhold Kainhofer <reinhold <at> kainhofer.com> writes:
LGTM
 
> Am Freitag, 5. August 2011, 21:22:49 schrieb Keith OHara:
> 
> > The entire source tree generates only seven warnings, all but one of which
> > point to code we could write more clearly.
> 
> Ah, great. Will you try to fix them?

You beat me to it :-)
I'll clean up the .ll, unless Neil has already done that.

http://codereview.appspot.com/4854049/diff/2001/lily/glissando-engraver.cc
File lily/glissando-engraver.cc (right):

http://codereview.appspot.com/4854049/diff/2001/lily/glissando-engraver.cc#ne...
lily/glissando-engraver.cc:124: if (n1 < 0 || n2 < 0 || unsigned(n1) >=
note_heads.size ())
just confirming, that was what the compiler was doing, per the C++ standard.

http://codereview.appspot.com/4854049/diff/2001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/4854049/diff/2001/lily/skyline.cc#newcode409
lily/skyline.cc:409: // FIXME: This has hardcoded logic, assuming a == AXIS_X!
 assert (AXIS_X == a);
instead?

http://codereview.appspot.com/4854049/diff/5001/lily/lyric-combine-music-iter...
File lily/lyric-combine-music-iterator.cc (right):

http://codereview.appspot.com/4854049/diff/5001/lily/lyric-combine-music-iter...
lily/lyric-combine-music-iterator.cc:224:
Lyric_combine_music_iterator::check_new_context (SCM /*sev*/)
On 2011/08/09 21:20:29, Carl wrote:
> Why not /*SCM sev*/ with a FIXME?

the function is declared with DECLARE_LISTENTER so it has to take a stream
event, even if it doesn't use it.

[Graham Percival (old account), 2011-08-10 05:19:42]

http://codereview.appspot.com/4854049/diff/5001/input/regression/completion-h...
File input/regression/completion-heads-factor.ly (right):

http://codereview.appspot.com/4854049/diff/5001/input/regression/completion-h...
input/regression/completion-heads-factor.ly:12: \paper { #(set-paper-size "a6")
}
is this part of the compiler warnings patch?

http://codereview.appspot.com/4854049/diff/5001/lily/glissando-engraver.cc
File lily/glissando-engraver.cc (right):

http://codereview.appspot.com/4854049/diff/5001/lily/glissando-engraver.cc#ne...
lily/glissando-engraver.cc:124: if (n1 < 0 || n2 < 0 || unsigned(n1) >=
note_heads.size ())
could we also get some parentheses around the cases?  If/when we add
  -Wextra
we'll get warnings about those anyway.

Also, my initial preference would be to case note_heads.size() to int rather
than n1 to unsigned, but I confess that I can't think of an actual rule as to
why that is.  It's just from seeing things done that way more often than the
alternative.

[Graham Percival (old account), 2011-08-10 05:20:48]

On 2011/08/10 05:19:42, Graham Percival wrote:
> input/regression/completion-heads-factor.ly:12: \paper { #(set-paper-size
"a6")
> }
> is this part of the compiler warnings patch?

sorry, ignore that, I only just looked at the changelog message.

[pkx166h, 2011-08-10 21:21:09]

Passes make but there are some reg test differences

see

http://code.google.com/p/lilypond/issues/detail?id=804#c6

[benko.pal, 2011-08-11 10:04:49]

generally signed-unsigned problems are better solved
by casting to size_t, not to unsigned - size_t works
equally well in 32 bit and 64 bit mode.  (even better,
some variables can be size_t instead of int.)

p

[Reinhold, 2011-08-15 13:44:58]

http://codereview.appspot.com/4854049/diff/5001/lily/glissando-engraver.cc
File lily/glissando-engraver.cc (right):

http://codereview.appspot.com/4854049/diff/5001/lily/glissando-engraver.cc#ne...
lily/glissando-engraver.cc:124: if (n1 < 0 || n2 < 0 || unsigned(n1) >=
note_heads.size ())
On 2011/08/10 05:19:42, Graham Percival wrote:
> Also, my initial preference would be to case note_heads.size() to int rather
> than n1 to unsigned, but I confess that I can't think of an actual rule as to
> why that is.  It's just from seeing things done that way more often than the
> alternative.

I deliberately casted n1 to unsigned, because we already checked that its value
is positive, so the cast will never ever change the value used in the check.
Casting note_head.size() to int might change the comparison and yield wrong
results (although, if we have more than 2^15 noteheads in a glissande, we
probably will have more serious problems, like memory issues; but still, casting
to unsigned is cleaner IMO).