more comments

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

[janek, 2013-08-20 09:53:28]

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#n...
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.

[fred1995, 2013-08-20 10:58:03]

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#n...
flower/include/std-string.hh:40: typedef size_t ssize; ///< I believe it is in
some c... header.
> 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.

No: ssize_t is the signed version of size_t used for file streams. It is
important that is remains signed. I looked at it and saw that it is used because
we use the C FILE for i/o which we shouldn't use in C++. The equivalent in
standard C++ is streamsize.

[janek, 2013-08-20 11:13:33]

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#n...
flower/include/std-string.hh:40: typedef size_t ssize; ///< I believe it is in
some c... header.
On 2013/08/20 10:58:03, fred1995 wrote:
> > 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.
> 
> No: ssize_t is the signed version of size_t used for file streams. It is
> important that is remains signed. I looked at it and saw that it is used
because
> we use the C FILE for i/o which we shouldn't use in C++. The equivalent in
> standard C++ is streamsize.

ok, i see now.  So, ssize_t should remain, but renaming size_t
to ssize doesn't make any sense.

Janek

[fred1995, 2013-08-20 11:21:38]

> ok, i see now.  So, ssize_t should remain, but renaming size_t
> to ssize doesn't make any sense.

That's right. Frédéric

[fred1995, 2013-08-20 18:58:38]

https://codereview.appspot.com/13051044/diff/1/lily/include/tie-column-format.hh
File lily/include/tie-column-format.hh (right):

https://codereview.appspot.com/13051044/diff/1/lily/include/tie-column-format...
lily/include/tie-column-format.hh:27: 
agreed: I checked that none of the function declared in this header are called.
I also removed the file. Only 2 files were including this header but without
using it. I will post a patch.

[fred1995, 2013-08-20 19:51:53]

https://codereview.appspot.com/13051044/diff/1/lily/misc.cc
File lily/misc.cc (left):

https://codereview.appspot.com/13051044/diff/1/lily/misc.cc#oldcode116
lily/misc.cc:116: }
it is just a modified inverse function so that f(0)=1 instead of +inf and
f(threshold)=0 instead of f(+inf)=0.
epsilon is used to control the speed at which the function decreases: the
smaller epsilon, the quicker the function decreases. When epsilon = +inf, the
function is linear.
Note that we could add:
assert(threshold>0.);
assert(epsilon>0.);

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

https://codereview.appspot.com/13051044/diff/1/lily/tie-configuration.cc#newc...
lily/tie-configuration.cc:128: *
changing 'string' to 'const string&' where it makes sense is something I would
be good at.

[fred1995, 2013-08-21 18:45:02]

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

https://codereview.appspot.com/13051044/diff/1/lily/tie-configuration.cc#newc...
lily/tie-configuration.cc:128: *
I have started to prepare a patch. There are many locations concerned!