Re: Rewrite Skyline code (issue 547980044 by address@hidden)

[hanwenn]

https://codereview.appspot.com/547980044/diff/548010043/lily/skyline.cc
File lily/skyline.cc (right):

https://codereview.appspot.com/547980044/diff/548010043/lily/skyline.cc#newcode348
lily/skyline.cc:348: result.push_back (buildings->at (0));
On 2020/04/21 04:25:05, Dan Eble wrote:
> at() involves a boundary check that is not necessary here.  front() or
[0] would
> be better.

Done.

https://codereview.appspot.com/547980044/diff/548010043/lily/skyline.cc#newcode862
lily/skyline.cc:862: // testing
On 2020/04/21 04:25:05, Dan Eble wrote:
> I love the idea of testing, though I think we'll want to find a way to
avoid
> bloating LilyPond with test code.

I agree that this approach is unorthodox, but I there is little
alternative. The data structure under test is a smob, so testing without
setting up the GUILE infrastructure will be hard. For the amount of
unittests we currently have (ie. none), it is a lot of overhead to setup
an extra compile, integrate the testing into the build, and keep the
test framework in sync with the main code's GUILE integration. 

We could compile enclose the code with #ifndef NDEBUG and use the same
mechanism that we use to suppress assert statements (although I think
those are shipped in production code too?)

If we start having lots of unittests (which would be great!) we can
consider splitting of unittests is worth it.

https://codereview.appspot.com/547980044/