Issue 5310: find_top_context () maintenance [ABANDONED]

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

* Let find_top_context () deal with references instead of pointers
  because every non-empty tree has a top.

* Call find_top_context () rather than get_global_context () where the
  caller uses the result as a mere Context.

[dak, 2018-04-21 20:58:32]

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)
What problem are you trying to fix here?  find_top_context worked given a null
pointer before your change.  This is no longer the case afterwards since a
reference is guaranteed not to refer to a null vma.  And it was simpler to
understand and debug.

[Carl, 2018-04-22 01:18:22]

It seems like if there is a problem that this solves, there should be a
regression test that shows the problem and hence why this patch is needed.

[Dan Eble, 2018-04-22 13:19:37]

On 2018/04/22 01:18:22, Carl wrote:
> It seems like if there is a problem that this solves, there should be a
> regression test that shows the problem and hence why this patch is needed.

The problems this solves are semantic.  I should have classified this as
"maintainability" in the ticket; that's done now.

[Dan Eble, 2018-04-22 13:19:46]

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;
}

[Dan Eble, 2018-04-22 13:21:46]

On 2018/04/22 13:19:46, Dan Eble wrote:
> of calls in some scope: if (Context *c = find_whatever(...)) { a(*p); b(*p);
> c(*p); }.

of course I meant *c for *p

[dak, 2018-04-22 13:37:50]

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/22 13:19:45, Dan Eble wrote:
> 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.

But not in the LilyPond code base.  It does not pass Scheme-able values with
identity by reference as a rule.  Part of the reason is that the identity can be
verified by pointer comparison, and that is what eq? does in the Scheme world. 
In Scheme, pointers are a basic mechanism for passing information and ensuring
identity.  Changing pointers in an irregular pattern to references is breaking a
direct relation between value and identity handling between C++ and Scheme, a
relation that actually is relevant for the Boehm GC method used in Guile 2.0 for
dealing with garbage collection.

Exactly because we do things consistently so far and don't pass Scheme objects
with identity by reference, your example is not to the point: it would rather be

if (Context *c = find_whatever(...)) { a(p); b(p); c(p); }

So again I don't see what problem you are trying to solve.  Particularly since
the interface between SCM and C++ involves using pointers and then converting
into references via *p seems spurious and only looks as if there cannot be null
pointers.

Basically you want to abuse reference notation as some kind of value passing
contract, mixing up semantics and syntax in the process.

[Dan Eble, 2018-04-22 13:56:01]

On 2018/04/22 13:37:50, dak wrote:
> So again I don't see what problem you are trying to solve.

1. find_something(...)->blahblah()
I have no idea whether that will call blahblah() with a valid instance of
something.

2. find_something(...).blahblah()
I know will call blahblah() with a valid instance of something.

> Basically you want to abuse reference notation as some kind of value passing
> contract, mixing up semantics and syntax in the process.

I don't understand your concerns, but this change is not that important to me,
so I will withdraw it.  "If it can't be null, use a reference" has been the rule
of thumb at work for almost two decades, but LilyPond is older than that, so I
will try to cope with it.

> Part of the reason is that the identity can be
> verified by pointer comparison, and that is what eq? does in the Scheme world.


I would like to understand this statement a little better for my own benefit. 
The address of a referenced object can be obtained with operator &.  It isn't
lost.

[dak, 2018-04-22 14:26:45]

On 2018/04/22 13:56:01, Dan Eble wrote:
> On 2018/04/22 13:37:50, dak wrote:
> > So again I don't see what problem you are trying to solve.
> 
> 1. find_something(...)->blahblah()
> I have no idea whether that will call blahblah() with a valid instance of
> something.

C++ does not allow "this" to be 0, so you have the syntactic guarantee that it
will not be 0 or you will have undefined behavior.

> 2. find_something(...).blahblah()
> I know will call blahblah() with a valid instance of something.

*p is not a valid reference if p is 0, so you have the syntactic guarantee that
it will not be 0 or you will have undefined behavior.

Basically you don't change when behavior is undefined but still mess up the
syntax and the relation between SCM and C++ pointers.

> > Basically you want to abuse reference notation as some kind of value passing
> > contract, mixing up semantics and syntax in the process.
> 
> I don't understand your concerns, but this change is not that important to me,
> so I will withdraw it.  "If it can't be null, use a reference" has been the
rule
> of thumb at work for almost two decades, but LilyPond is older than that, so I
> will try to cope with it.

When your work location confuses reference notation with a value passing
contract, that's not a problem of the age of LilyPond.

"This cannot be zero because surely Peter would not introduce undefined behavior
lightly." is not a value passing contract.

  assert (p);

is explicit if you need that and does not require messing with references.  The
main purpose of references is to avoid spurious temporary copies in large-size
structures used in value context or for user-defined operators.

Not for managing heap-only objects with identity rather than value, and things
like contexts _are_ carrying identity.

> > Part of the reason is that the identity can be
> > verified by pointer comparison, and that is what eq? does in the Scheme
world.
> 
> 
> I would like to understand this statement a little better for my own benefit. 
> The address of a referenced object can be obtained with operator &.  It isn't
> lost.

But it makes the address a property of the object rather than the fundamental
entity conveying its identity.  And you can take the address of temporary
copies/values/variable as well in that manner: depending on what copy
contructors and whatever do, you don't get the address of _the_ object but just
_some_ address of something having the same value.  It's not something you would
want to compare or sort on.

There is a difference between const references and variable references involved
here as well (variable references are not magically attaching themselves to
temporary copies, but const references may do so).  When passing around pointers
rather than references, there are no copies created silently either way.

Basically the semantic guarantees you are aiming for here are just not there
either way (the undefined behavior starts at a different point but is still
undefined) but the semantics of references match worse for our identity-carrying
heap objects that those of pointers.

[Dan Eble, 2018-04-23 23:55:44]

David, would you accept a template class wrapping a non-null pointer, perhaps
non_null_ptr<T>?  Conversion to a raw pointer would be implicit, but
construction from a raw pointer would be explicit.  The person constructing such
a pointer is responsible for ensuring that it is not null.  The person who uses
such a pointer trusts that it is not.

That would retain the pointers that you value and add the nullness information
that I value.

[dak, 2018-04-25 10:44:22]

On 2018/04/23 23:55:44, Dan Eble wrote:
> David, would you accept a template class wrapping a non-null pointer, perhaps
> non_null_ptr<T>?  Conversion to a raw pointer would be implicit, but
> construction from a raw pointer would be explicit.  The person constructing
such
> a pointer is responsible for ensuring that it is not null.  The person who
uses
> such a pointer trusts that it is not.
> 
> That would retain the pointers that you value and add the nullness information
> that I value.

Please note that all of your proposals so far do _not_ provide the nullness
information that you value in any manner: previously undefined behavior would
continue to be undefined behavior and would just look differently.

Either way, to make this work at all in a mechanical manner, you need to be
consistent in the value creation chain.  That is, you cannot start from unsmob
(which may return null or not which is also used as a type check) but would need
to start with a different function delivering then either a reference or the
kind of null-checked pointer you now suggest.  Every further use must then
retain this particular type, not allowing for a piecemeal conversion of the code
conventions.  The latter would at least have the advantage of somewhat definable
semantics (like implicit conversion to pointer) making for a somewhat softer
conversion and giving somewhat more dependable semantics than just replacing
pointers by references.

It still has the disadvantage of adding a new class as baggage, making it harder
for newcomers interpreting code.  In particular the necessity of overloading *,
&, and -> operators raises the amount of knowledge required for maintenance and
debugging.  I am rather doubtful that this would be a good tradeoff so I would
not want to encourage you to indulge in a significant amount of work here which
I may not want to see added to the code base after all.

[Dan Eble, 2018-04-25 23:05:08]

On 2018/04/25 10:44:22, dak wrote:

> Please note that all of your proposals so far do _not_ provide the nullness
> information that you value in any manner: previously undefined behavior would
> continue to be undefined behavior and would just look differently.

Yes, but what I believe you are overlooking is the value to the reader.  If a
developer explicitly constructs a sure_ptr<> (or whatever) without checking that
the pointer he is providing is not null, the blame for undefined behavior is
easy to place.  The intent is clear that such a pointer should not be null, so
the failure to check before construction is what needs to be fixed.

With raw pointers, when faced with code like
get_something(x)->do_something_with_it(...), it can require a lot more effort to
satisfy oneself that the person who wrote it was justified in not checking
whether the result of get_something(x) is null.  Worse, maybe it was fine at the
time, but someone goes and changes get_something(x) so that it can return null
rarely enough that nobody notices for a while.

> Either way, to make this work at all in a mechanical manner, you need to be
> consistent in the value creation chain. . . . Every further use must then
> retain this particular type, not allowing for a piecemeal conversion of the
code
> conventions.

Now you're speaking my language.

> It still has the disadvantage of adding a new class as baggage, making it
harder
> for newcomers interpreting code.  In particular the necessity of overloading
*,
> &, and -> operators raises the amount of knowledge required for maintenance
and
> debugging.  I am rather doubtful that this would be a good tradeoff so I would
> not want to encourage you to indulge in a significant amount of work here
which
> I may not want to see added to the code base after all.

If you're at least open to reviewing something concrete, I'll put something
together, but not today.  Thanks for your time so far.

[dak, 2018-04-29 21:23:18]

On 2018/04/25 23:05:08, Dan Eble wrote:
> On 2018/04/25 10:44:22, dak wrote:
> > It still has the disadvantage of adding a new class as baggage, making it
harder
> > for newcomers interpreting code.  In particular the necessity of overloading
*,
> > &, and -> operators raises the amount of knowledge required for maintenance
and
> > debugging.  I am rather doubtful that this would be a good tradeoff so I
would
> > not want to encourage you to indulge in a significant amount of work here
which
> > I may not want to see added to the code base after all.
> 
> If you're at least open to reviewing something concrete, I'll put something
> together, but not today.  Thanks for your time so far.

I have forgotten one consideration: we convert pointers along the class
hierarchy (like between Grob * and Item *) and a similar conversion of a
user-created type would not likely work in a comparable manner.

[Dan Eble, 2019-12-12 23:59:30]

Coming back to this after a long time...

I intend to abandon the pointer/reference changes and submit a new review
covering just the second point: "Call find_top_context () rather than
get_global_context () where the
  caller uses the result as a mere Context."

Those interested in the pointer/reference issue can find some discussion in the
C++ Core Guidelines maintained by Stroustrup and Sutter:

    F.60: Prefer T* over T& when “no argument” is a valid option
    https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-ptr-ref

    F.44: Return a T& when copy is undesirable and “returning no object” isn’t
needed
    https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-return-ref

There is also a related "Guidelines Support Library" that provides a not_null<>
wrapper that limits the use of pointers that are expected not to be null.  It is
licensed under the MIT license and assumes C++14 support.

[Dan Eble, 2019-12-13 00:01:01]

> There is also a related "Guidelines Support Library" that provides a
not_null<>

https://github.com/Microsoft/GSL

[Dan Eble, 2019-12-13 00:11:26]

New review: https://codereview.appspot.com/565320043/