Code review - Issue 341150043: Issue 5310: find_top_context () maintenance [ABANDONED]https://codereview.appspot.com/2019-12-13T00:11:26+00:00rietveld
Message from unknown
2018-04-21T20:38:54+00:00Dan Ebleurn:md5:43814cb020cd50f4a554b21865f15e25
Message from nine.fierce.ballads@gmail.com
2018-04-21T20:39:49+00:00Dan Ebleurn:md5:df0377a304ce74a4765e73f12e085b6d
Message from dak@gnu.org
2018-04-21T20:58:32+00:00dakurn:md5:9dfc0ca192cc82e9e6f39b58467e6693
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.
Message from Carl.D.Sorensen@gmail.com
2018-04-22T01:18:22+00:00Carlurn:md5:a16a688dee1f5a02972a53d7cee07b62
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.
Message from nine.fierce.ballads@gmail.com
2018-04-22T13:19:37+00:00Dan Ebleurn:md5:1b8769a8d382d23250610a2cc2f57f94
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.
Message from nine.fierce.ballads@gmail.com
2018-04-22T13:19:46+00:00Dan Ebleurn:md5:550096d6adcaff479cecc7b486f0d098
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;
}
Message from nine.fierce.ballads@gmail.com
2018-04-22T13:21:46+00:00Dan Ebleurn:md5:c22996e1642e0309f8ba87cde54e6a26
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
Message from dak@gnu.org
2018-04-22T13:37:50+00:00dakurn:md5:1f4555a71ae8c5d33488592971c3c5aa
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.
Message from nine.fierce.ballads@gmail.com
2018-04-22T13:56:01+00:00Dan Ebleurn:md5:dc699fcde90973ffa3346eacc4de341f
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.
Message from dak@gnu.org
2018-04-22T14:26:45+00:00dakurn:md5:d19509c83c8963023847ed569c515b66
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.
Message from nine.fierce.ballads@gmail.com
2018-04-23T23:55:44+00:00Dan Ebleurn:md5:8d1fcdd3e01e77e2686bf53ba45c8300
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.
Message from dak@gnu.org
2018-04-25T10:44:22+00:00dakurn:md5:6672e801d2e69d1d398d097ffd629258
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.
Message from nine.fierce.ballads@gmail.com
2018-04-25T23:05:08+00:00Dan Ebleurn:md5:82153633a841abcdda118c987bf4d658
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.
Message from dak@gnu.org
2018-04-29T21:23:18+00:00dakurn:md5:af4ac5c29fe4ca865dd139d905c518fd
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.
Message from nine.fierce.ballads@gmail.com
2019-12-12T23:59:30+00:00Dan Ebleurn:md5:18559b4f3c452a8dabfb0779cd30c323
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.
Message from nine.fierce.ballads@gmail.com
2019-12-13T00:01:01+00:00Dan Ebleurn:md5:cb519a45a7851a0819c0400c2bfdd716
> There is also a related "Guidelines Support Library" that provides a not_null<>
https://github.com/Microsoft/GSL
Message from nine.fierce.ballads@gmail.com
2019-12-13T00:11:26+00:00Dan Ebleurn:md5:39ba891ebfa4abd35062e23dff161c7e
New review: https://codereview.appspot.com/565320043/