Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by address@hidden)

[hanwenn]

On 2020/01/29 11:44:57, dak wrote:
> > BTW - I don't want to tell a C++ expert how to use the language in
general. If
> > we were in an alternate reality  where we could start from scratch
we could
> > reconsider the decision to not use non-const references in structs
and
> function
> > arguments. As it is however, any that we have are most likely errors
that we
> > should correct. Check
> 
> Errors mean non-intentional things. 

This may predate you, but the decision to not store references was
intentional,
exactly because storing NULL in them is very useful.  If you have a
reference,
it has to be initialized in the constructor, and this introduces a lot
of boilerplate
because you have to pass the non-null reference across constructors in
the class
hierarchy. 

Imagine adding something to the Prob class. If it is a reference, you'll
now have 
to update a dozen classes just so it compiles again.

Maybe LilyPond has passed the phase that we need to refactorings, but I
remember 
refactoring constructors being a major PITA. Hence, no references. 

> My own take here is that we would not want
> to use references on things that may be used as SCM values, so things
like
> 
> Grob &bla = *unsmob<Grob> (sbla);
> 
> would be quite undesirable.  However, for code that has no Schemish
> implications, I find it perfectly fine to use C++ references in the
manner they
> are intended to be used.  There has been a recent push to settle on
C++11 as a
> programming standard to adhere to in order to be more friendly to
newcomers, and
> it seems sort of pointless to promote C89 standards to go alongside. 
That makes
> people less, not more confident in what they may rely on using.

People usually make changes by copy & pasting existing code, and then
adapting it to their needs. If there are two ways of structuring the
code, this makes things more confusing.

> > grep --color -nH  -e '&' lily/include/*h|grep -v const
> > 
> > Also, it is rare to check incoming parameters for nullness in
implementation.
> 
> I am not exactly sure I'd call that a feature: we had crashes because
of it. 
> Some trampolining code now has the requisite assertions inside since
one cannot
> perform those checks too late: GCC optimises checks like "if (!this)"
away these
> days.

It is correct that using references makes it harder to do null-pointer
derefs, and 
I am not saying the lack of null checks are a desirable feature.

what I am trying to say: 

For reading the code, it is important for the source code to be
consistent with itself. This is also why we have automated formatting.  

The current code overwhelmingly disavows references. The 2 remaining
uses (this being one) stand out like a sore thumb.

If anyone wants to modernize the source code to introduce references, I
think this should be done with an overall plan of how this will become a
consistent idiom. I personally think doing so does not solve pressing
problems, but as I won't be doing the work that doesn't bother me so
much.

My proposal is to go ahead with this change, so I can go then go on with

https://github.com/hanwen/lilypond/commit/599878a08c18323810aaa1dee3bf4d9b208e223c

which is based on this one. 

If anyone has a plan for changing pointers to references globally, I am
happy to comment on it in a different review thread.

Meta question: if I would keep shut for 7 days, is this a change that
would go in based on "countdown" ?


https://codereview.appspot.com/577410045/