Issue 5732: Use unique_ptr in layout code

https://sourceforge.net/p/testlilyissues/issues/5732/

Use std::unique_ptr instead of manually managed raw pointers.

I suggest beginning the review in accidental-placement.cc  The
algorithm in stagger_apes () took me the most time to come to terms
with.  With unique_ptr, it's much easier to be confident that there
isn't a memory leak there.

unique_ptr deletes its object when it goes out of scope.  Transferring
ownership of the object to another instance of unique_ptr is done with
std::move().  A unique_ptr can not be copied, but one can get an alias
(a raw pointer) from it with p.get().

Using unique_ptr makes the flower junk_pointers() function
unnecessary.  I can't remove junk_pointers() yet because MIDI code
still uses it, and I don't want to wade into that high water now.

I'm aware that some lines are longer than 80 characters.  I plan to
try clang-format after Han-Wen's config file makes it through the
countdown.

[hahnjo, 2020-02-03 17:57:53]

Just scanned the code, don't take this as a full review.

In general I'm a huge fan of unique_ptrs, it makes memory ownership semantically
clear.

https://codereview.appspot.com/573500043/diff/561420043/lily/beam-quanting.cc
File lily/beam-quanting.cc (right):

https://codereview.appspot.com/573500043/diff/561420043/lily/beam-quanting.cc...
lily/beam-quanting.cc:1049: configs.clear ();
I think you don't need this: If a vector goes out of scope, it will call the
destructor of all elements still present.

https://codereview.appspot.com/573500043/diff/561420043/lily/system-start-del...
File lily/system-start-delimiter-engraver.cc (right):

https://codereview.appspot.com/573500043/diff/561420043/lily/system-start-del...
lily/system-start-delimiter-engraver.cc:149: (new Bracket_nesting_staff (0)));
Can you check if
children_.emplace_back (new Bracket_nesting_staff (0));
works? This would be much neater

https://codereview.appspot.com/573500043/diff/561420043/lily/system-start-del...
lily/system-start-delimiter-engraver.cc:230: (new Bracket_nesting_staff (0)));
likewise

[dak, 2020-02-03 18:01:09]

Stupid question: unique_ptr has the purpose of deallocating memory when the last
reference is gone.  But we have an entire Scheme allocation system exactly for
that purpose for which we are already paying the price in overhead.  Any chance
this can be usefully tracked in the SCM scheme of things?

[hahnjo, 2020-02-03 18:13:00]

On 2020/02/03 18:01:09, dak wrote:
> Stupid question: unique_ptr has the purpose of deallocating memory when the
last
> reference is gone.  But we have an entire Scheme allocation system exactly for
> that purpose for which we are already paying the price in overhead.  Any
chance
> this can be usefully tracked in the SCM scheme of things?

Why would we want to use GC more than needed? Any marking algorithm will always
be slower than a local variable going out of scope.

[Dan Eble, 2020-02-03 18:27:23]

On 2020/02/03 18:01:09, dak wrote:
> Stupid question: unique_ptr has the purpose of deallocating memory when the
last
> reference is gone.

When the single, owning reference is gone.  It's not reference counted.

> But we have an entire Scheme allocation system exactly for
> that purpose for which we are already paying the price in overhead.  Any
chance
> this can be usefully tracked in the SCM scheme of things?

Do you mean turn Accidental_placement_entry etc. into Smobs, call a Guile
function to sort a vector, etc.?  It seems possible, but is not something that
piques my interest.

To the point of overhead: you're not paying anything for unique_ptr if your
baseline for comparison is raw-pointer code that explicitly calls delete
everywhere it needs to be called to avoid leaks.  unique_pointer might be slower
than leaky code.

[dak, 2020-02-03 18:27:27]

On 2020/02/03 18:13:00, hahnjo wrote:
> On 2020/02/03 18:01:09, dak wrote:
> > Stupid question: unique_ptr has the purpose of deallocating memory when the
> last
> > reference is gone.  But we have an entire Scheme allocation system exactly
for
> > that purpose for which we are already paying the price in overhead.  Any
> chance
> > this can be usefully tracked in the SCM scheme of things?
> 
> Why would we want to use GC more than needed? Any marking algorithm will
always
> be slower than a local variable going out of scope.

unique_ptr has bookkeeping overhead.  The marking algorithm runs anyway and has
no memory overhead.  I have not looked at the actual patch yet, but unique_ptr
really is doing pretty much the same job as SCM except that it acts with no
delay, so if it is important that the destructors are called right away when the
last reference is gone, SCM will not do the job.

[dak, 2020-02-03 18:41:53]

On 2020/02/03 18:27:27, dak wrote:
> On 2020/02/03 18:13:00, hahnjo wrote:
> > On 2020/02/03 18:01:09, dak wrote:
> > > Stupid question: unique_ptr has the purpose of deallocating memory when
the
> > last
> > > reference is gone.  But we have an entire Scheme allocation system exactly
> for
> > > that purpose for which we are already paying the price in overhead.  Any
> > chance
> > > this can be usefully tracked in the SCM scheme of things?
> > 
> > Why would we want to use GC more than needed? Any marking algorithm will
> always
> > be slower than a local variable going out of scope.
> 
> unique_ptr has bookkeeping overhead.  The marking algorithm runs anyway and
has
> no memory overhead.  I have not looked at the actual patch yet, but unique_ptr
> really is doing pretty much the same job as SCM except that it acts with no
> delay, so if it is important that the destructors are called right away when
the
> last reference is gone, SCM will not do the job.

Sorry, I confused this with shared_ptr.  unique_ptr just has compilation
overhead, no runtime or memory overhead.  Nothing to be seen here, I withdraw my
objection.  This is definitely not something where SCM would solve the same
task.

[Dan Eble, 2020-02-03 18:53:45]

Thanks for the reviews, g

https://codereview.appspot.com/573500043/diff/561420043/lily/beam-quanting.cc
File lily/beam-quanting.cc (right):

https://codereview.appspot.com/573500043/diff/561420043/lily/beam-quanting.cc...
lily/beam-quanting.cc:1049: configs.clear ();
On 2020/02/03 17:57:52, hahnjo wrote:
> I think you don't need this: If a vector goes out of scope, it will call the
> destructor of all elements still present.

I didn't want to investigate the consequences of deferring deletion to the end
of the scope.

https://codereview.appspot.com/573500043/diff/561420043/lily/system-start-del...
File lily/system-start-delimiter-engraver.cc (right):

https://codereview.appspot.com/573500043/diff/561420043/lily/system-start-del...
lily/system-start-delimiter-engraver.cc:149: (new Bracket_nesting_staff (0)));
On 2020/02/03 17:57:52, hahnjo wrote:
> Can you check if
> children_.emplace_back (new Bracket_nesting_staff (0));
> works? This would be much neater

It compiles, but it raises its own flag to reviewers because it does not show
that the new object is managed by a smart pointer.

If I had C++14, I would have used std::make_unique<Bracket_nesting_staff> (0) to
avoid repeating the class name.  We can define our own make_unique if we want
to, but I don't think it should be done in this patch.

[Dan Eble, 2020-02-03 18:55:44]

On 2020/02/03 18:53:45, Dan Eble wrote:
> Thanks for the reviews, g

... guys.  (A stray keystroke caused a premature submission.)

[hahnjo, 2020-02-03 18:58:25]

On 2020/02/03 18:53:45, Dan Eble wrote:
>
https://codereview.appspot.com/573500043/diff/561420043/lily/system-start-del...
> File lily/system-start-delimiter-engraver.cc (right):
> 
>
https://codereview.appspot.com/573500043/diff/561420043/lily/system-start-del...
> lily/system-start-delimiter-engraver.cc:149: (new Bracket_nesting_staff (0)));
> On 2020/02/03 17:57:52, hahnjo wrote:
> > Can you check if
> > children_.emplace_back (new Bracket_nesting_staff (0));
> > works? This would be much neater
> 
> It compiles, but it raises its own flag to reviewers because it does not show
> that the new object is managed by a smart pointer.

Fair point. That was already settled in my head, but probably only because of
the context.

> If I had C++14, I would have used std::make_unique<Bracket_nesting_staff> (0)
to
> avoid repeating the class name.  We can define our own make_unique if we want
> to, but I don't think it should be done in this patch.

Agreed.