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

[dak]

On 2020/01/30 09:54:48, hanwenn wrote:
> 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.

That assumes that every variable is created equal, and every purpose
is the same.

> 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.

That assumes that references are being used within a class hierarchy.


> Imagine adding something to the Prob class.

We are not talking about the Prob class.  We are not looking for
one-size-fits-all solutions, but for a solution matching the problem.

> If it is a reference, you'll now have to update a dozen classes just
> so it compiles again.

There is no reason whatsoever that the existing constructors in the
Prob class should not cater for initializing the reference.

I agree that references in data structures can be a problem, and very
much so if the data structure is supposed to have a persistent life
time, like basically all Smobs do, because references have a life time
tied to the referenced objects.

But that's not what we are talking about here: here we have a
parameter packet with a life-time exactly bounded by the call.

Basically you state that we should not be using tools appropriate to
the job we need to solve, but restrict ourselves to a common
denominator.  With a language as large as C++, which has grown to its
size exactly because there are a lot of things not solved well by
existing tools, that is not going to be a good fit.

We recently decided to bump our C++ standard up to C++11 in order to
enable a more idiomatic programming style for newcomers, and yet you
argue for restricting use of the most basic C++ programming techniques
regardless of the context they are used in.

Prescribing a rigid mixture of archaic and modern is not likely to
increase the attractiveness for LilyPond contributors in general.

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

This sounds like "my way or the highway".  That is a valid and in some
manners efficient way of structuring a project top-down, but at the
current point of time LilyPond's leadership model is more based on
consensus-finding.  If changing that is desired in order to get rid of
cumbersome discussions, I think it should at least be put up to
debate.

> > 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.

C++ is not Lua.  It just doesn't have one-size-fits-all constructs, so
there will always either be a considerable number of tools in play, or
the code becomes non-idiomatic and cumbersome to work with.

> what I am trying to say: 
> 
> For reading the code, it is important for the source code to be
consistent with
> itself.

C++ is what it is, and it is what we have to work with.  Consistency
for me does not mean that I refrain from using references where
appropriate but that I refrain from littering code all over the place,
like with copy&paste.  For example, consistency for me means that all
accesses to Scheme data structures now without exception run through
smobs.hh and associated header and C files.  That is why disabling
Scheme marking passes in all of LilyPond can be done by outcommenting
a single line in all of the source code.  It is also why changing the
overall GC schemes is feasible.

> This is also why we have automated formatting.

Nope.  We have automated formatting so that people reading the code do
not get irritated by _gratuitous_ differences, asking themselves "why
is this different?".  That is different from having differences with a
rationale.

I am not beholden to this particular code in any manner, but I think
that trying to stamp out appropriate use of long established C++
techniques in order to make the code look more uniform does not really
make for a sensible plan in combination with demanding a modernization
of development.

I readily agree that for the dynamic life times of Scheme objects,
references are rarely useful because of tying the life time of the
class/struct containing the reference to that of the referenced
object/variable.  They make sense mainly tied to local variables in
function scope, in structures that live and die in the same scope.

Which quite often means that they make sense as function parameter and
return types, and in structs/classes used exclusively for that
purpose.

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

There is no such plan, but I don't see that a plan for changing
references to pointers globally makes sense either.  They are
different tools for different purposes.

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

Open questions can put a change on Needs_work , depending on the
impression people have about the change.  I am not particularly
invested in keeping this reference in so I probably would not veto it,
but it certainly is appropriate to use for the purpose it has been
used here, and I don't see the point of promoting more modern C++
styles while ruling out long established techniques that cannot be
avoided (like in the arguments of copy constructors) anyway.

I don't think that the goal of getting new contributors is served by
turning the clock both back and forward at the same time.  It will
also not be served by having endless discussions all the time, so I
suggest we just have a vote of whether we want to disallow using
references in the code base in general and move on from there.


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