more comments (issue 13051044)

[janek . lilypond]

Reviewers: ,

Message:
Adding Frederic's comment and answering two of them.

Janek


https://codereview.appspot.com/13051044/diff/1/flower/include/std-string.hh
File flower/include/std-string.hh (right):

https://codereview.appspot.com/13051044/diff/1/flower/include/std-string.hh#newcode40
flower/include/std-string.hh:40: typedef size_t ssize; ///< I believe it
is in some c... header.
Frederic wrote:
> size_t is in <cstddef> but not ssize.
> The problem here is that size_t is still used 106 times so this
> doesn't look consistent. Why not removing ssize and use only size_t
> which is standard?

Indeed, this looks messy.  in addition to these two, there's ssize_t.
I think that using just size_t is what we should do.

https://codereview.appspot.com/13051044/diff/1/lily/engraver.cc
File lily/engraver.cc (right):

https://codereview.appspot.com/13051044/diff/1/lily/engraver.cc#newcode119
lily/engraver.cc:119: #endif
Frederic wrote:
file, line and fun are used only in debug mode. Therefore in release
mode, the compiler will issue a warning (unused variables). The proper
way to remove the warning is to remove the name of the unused
variables as done with 'name' but then the code won't compile in debug
mode... These lines are skipped by the compiler and the warning
disappears.

Personnaly, I prefer the following solution:
#ifdef NDEBUG
 Engraver::internal_make_grob (SCM symbol,
                               SCM cause,
                               char const * /* name */,
                               char const * /* file */,
                               int /* line */,
                               char const * /* fun */)
#else
 Engraver::internal_make_grob (SCM symbol,
                               SCM cause,
                               char const * /* name */,
                               char const *file,
                               int line,
                               char const *fun)
#endif

but it takes more lines...

https://codereview.appspot.com/13051044/diff/1/lily/tie.cc
File lily/tie.cc (right):

https://codereview.appspot.com/13051044/diff/1/lily/tie.cc#newcode112
lily/tie.cc:112: */
Frederic wrote:
> Do you mean that head(me, d) always returns nullptr?

I think it's because span->get_bound will get us the head if it exists.

Description:
more comments


more tie comments


another tie comment


tie comment: very ugh


jakis bzdet


tie comment: specification


tie comment answer


tie comments: another portion


moar komentaże


tie comments: more


tie comments: high WTF factor


first tie comments

Please review this at https://codereview.appspot.com/13051044/

Affected files:
  M flower/include/offset.hh
  M flower/include/std-string.hh
  M flower/warn.cc
  M lily/engraver.cc
  M lily/grob-property.cc
  M lily/grob-scheme.cc
  M lily/include/grob.hh
  M lily/include/main.hh
  M lily/include/tie-column-format.hh
  M lily/include/tie-configuration.hh
  M lily/include/tie-formatting-problem.hh
  M lily/include/tie-specification.hh
  M lily/include/tie.hh
  M lily/lily-guile.cc
  M lily/misc.cc
  M lily/program-option-scheme.cc
  M lily/staff-symbol-referencer.cc
  M lily/tie-column.cc
  M lily/tie-configuration.cc
  M lily/tie-engraver.cc
  M lily/tie-formatting-problem.cc
  M lily/tie-specification.cc
  M lily/tie.cc