Re: Issue 5310: find_top_context () maintenance (issue 341150043 by address@hidden)

[nine . fierce . ballads]

https://codereview.appspot.com/341150043/diff/1/lily/context.cc
File lily/context.cc (right):

https://codereview.appspot.com/341150043/diff/1/lily/context.cc#newcode723
lily/context.cc:723: find_top_context (Context &where)
On 2018/04/21 20:58:32, dak wrote:
> And it was simpler to understand and debug.

The opposite sense is deeply ingrained in me.

One of the things that has been frustrating me as I've been going
through the context code is finding callers that do not check returned
pointers before dereferencing them, and wondering whether it is really
the case that they will never be null.  When a function returns a
reference, I don't have to go digging into the implementation in an
attempt (usually in vain) to convince myself that a missing null check
is not a lurking bug.

find_top_context () was not a strong example of this, but it is was easy
to improve because given a context it will find a context 100% of the
time.  (Other find_... operations might not find anything, so a pointer
makes sense for them.)

An advantage of passing a reference in is that if it is already known in
the calling context that a pointer is not null, it is not checked again
in this function.  That advantage extends to a chain of calls taking
references.  The null check occurs once instead of at every level.  It
also extends to a series of calls in some scope: if (Context *c =
find_whatever(...)) { a(*p); b(*p); c(*p); }.

If you do not want me to use this style of design in LilyPond, I can try
to adapt, but I think it's an improvement.

Anyone who got into the position of wanting the old interface in
addition to the new one could easily restore it:

Context *find_top_context (Context *where)
{
    return where ? &find_top_context (*where) : 0;
}

https://codereview.appspot.com/341150043/