Clean up embedded scheme parsing/evaluation.

Clean up embedded scheme parsing/evaluation.
    
Renames and reorders functions to clarify the mechanism.
Separates input and output parameters. No consequential
functional changes.

[hanwenn, 2020-01-27 08:48:53]

this is preparation for
https://github.com/hanwen/lilypond/commit/599878a08c18323810aaa1dee3bf4d9b208...

[Dan Eble, 2020-01-27 13:39:31]

https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-sc...
File lily/include/parse-scm.hh (right):

https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-sc...
lily/include/parse-scm.hh:30: SCM parse_embedded_scheme (Input *i, bool safe,
Lily_parser *parser);
Changing Input& to Input* is more than cosmetic.  Input& requires an object, but
Input* admits a nullptr.  I'm concerned that I don't see that any checks have
been added before the pointer is dereferenced.

[hanwenn, 2020-01-28 22:06:33]

https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-sc...
File lily/include/parse-scm.hh (right):

https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-sc...
lily/include/parse-scm.hh:30: SCM parse_embedded_scheme (Input *i, bool safe,
Lily_parser *parser);
On 2020/01/27 13:39:31, Dan Eble wrote:
> Changing Input& to Input* is more than cosmetic.  Input& requires an object,
but
> Input* admits a nullptr.  I'm concerned that I don't see that any checks have
> been added before the pointer is dereferenced.

This code stores a reference, which is completely out of style in LilyPond.

In general, pointers are preferred in function signatures, so you can see that
the value is mutated in the call site. See also
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

What would we do in a check? Passing a null input is a programming error; if we
don't check, we'll generate a segfault and that seems appropriate for a
programming error.

[dak, 2020-01-28 22:37:19]

https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-sc...
File lily/include/parse-scm.hh (right):

https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-sc...
lily/include/parse-scm.hh:30: SCM parse_embedded_scheme (Input *i, bool safe,
Lily_parser *parser);
On 2020/01/28 22:06:32, hanwenn wrote:
> On 2020/01/27 13:39:31, Dan Eble wrote:
> > Changing Input& to Input* is more than cosmetic.  Input& requires an object,
> but
> > Input* admits a nullptr.  I'm concerned that I don't see that any checks
have
> > been added before the pointer is dereferenced.
> 
> This code stores a reference, which is completely out of style in LilyPond.

It's pretty untypical, not least because unsmobbing works on pointers.  That
being said, Input is likely the least coherently treated data structure in that
regard if I remember correctly, quite often getting passed by copy (as seen in
the signature of evaluate_embedded_scheme).  Part of the reason for this use may
be that for Scheme protection to set in, you need to actually smobify the
structure and then keep an SCM value around in the stack somewhere which may
prove inconvenient.  Having a local copy and passing a reference around is quite
more static regarding the life times.

Things go as far as Input() creating a "null input" without location data, so
the value "no input" is not actually represented by using a null pointer.

All that peculiarity of Input was not invented by me but originally there.  Part
of the reason may be that parser.yy needs a fixed data type for its @$ and
similar location data, so the C structure interface inherent there might have
been partly responsible for this somewhat peculiar usage.

I don't have a particular opinion on this usage myself but think that the
current use of a reference might have been done by me trying to keep with the
general spirit of how Input is getting used without losing sight of C++.

Not necessarily successfully so, but I doubt there were a lot of reviewers
advising me at the time.

[Dan Eble, 2020-01-28 23:38:24]

On 2020/01/28 22:06:33, hanwenn wrote:
>
> In general, pointers are preferred in function signatures, so you can see that
> the value is mutated in the call site. See also
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments

OK, they're preferred by one or more people at Google.  They're not preferred by
me, the people I've worked with, or the creator of C++:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-ptr-ref

> What would we do in a check? Passing a null input is a programming error; if
we
> don't check, we'll generate a segfault and that seems appropriate for a
> programming error.

Not seeing a satisfying way to handle a null pointer is a hint that using a
reference would be better.  You've preferred one of two kinds of run-time
errors, but the choice you've dismissed is a compile-time error when someone
tries to pass a pointer to a function that requires a reference.  A compile-time
error is an improvement over a run-time error, isn't it?

I don't want to spend a great deal time trying to change your mind, and I'm
certainly not going to tell a project founder what to do here, but I would rest
easier if there were at least a comment on the prototype that the pointer is not
allowed to be null.

[hanwenn, 2020-01-29 06:28:13]

comments

[hanwenn, 2020-01-29 06:36:07]

On 2020/01/28 23:38:24, Dan Eble wrote:
> On 2020/01/28 22:06:33, hanwenn wrote:
> >
> > In general, pointers are preferred in function signatures, so you can see
that
> > the value is mutated in the call site. See also
> > https://google.github.io/styleguide/cppguide.html#Reference_Arguments
> 
> OK, they're preferred by one or more people at Google.  They're not preferred
by
> me, the people I've worked with, or the creator of C++:
> https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-ptr-ref

I've never been a big fan of Stroustrup, so we are both coherent, at least :-) 

(see question 9
https://developers.slashdot.org/story/00/02/25/1034222/c-answers-from-bjarne-...)

> > What would we do in a check? Passing a null input is a programming error; if
> we
> > don't check, we'll generate a segfault and that seems appropriate for a
> > programming error.
> 
> Not seeing a satisfying way to handle a null pointer is a hint that using a
> reference would be better.  You've preferred one of two kinds of run-time
> errors, but the choice you've dismissed is a compile-time error when someone
> tries to pass a pointer to a function that requires a reference.  A
compile-time
> error is an improvement over a run-time error, isn't it?
>
> I don't want to spend a great deal time trying to change your mind, and I'm
> certainly not going to tell a project founder what to do here, but I would
rest
> easier if there were at least a comment on the prototype that the pointer is
not
> allowed to be null.

I reorganized a bit more; I hope you like it better now.

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
 
grep --color -nH  -e '&' lily/include/*h|grep -v const

Also, it is rare to check incoming parameters for nullness in implementation.

[dak, 2020-01-29 11:44:57]

On 2020/01/29 06:36:07, hanwenn 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.  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.

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

[dak, 2020-01-29 14:42:22]

https://codereview.appspot.com/577410045/diff/551410043/lily/parse-scm.cc
File lily/parse-scm.cc (right):

https://codereview.appspot.com/577410045/diff/551410043/lily/parse-scm.cc#new...
lily/parse-scm.cc:59: 
Pouring oil on the fire...  Rietveld highlighting indicates non-empty lines
consisting only of spaces.  Incidentally, those would already get highlighted
with

git log --check

[hanwenn, 2020-01-30 09:54:48]

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/599878a08c18323810aaa1dee3bf4d9b208...

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" ?

[hanwenn, 2020-01-30 09:55:07]

trailing space

[hanwenn, 2020-01-30 09:58:13]

https://codereview.appspot.com/577410045/diff/551410043/lily/parse-scm.cc
File lily/parse-scm.cc (right):

https://codereview.appspot.com/577410045/diff/551410043/lily/parse-scm.cc#new...
lily/parse-scm.cc:59: 
On 2020/01/29 14:42:22, dak wrote:
> Pouring oil on the fire...  Rietveld highlighting indicates non-empty lines
> consisting only of spaces.  Incidentally, those would already get highlighted
> with
> 
> git log --check

Done.

added a hook to my .emacs to delete them.

[dak, 2020-01-30 13:58:30]

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.

[dan_faithful.be, 2020-01-30 14:31:07]

On Jan 30, 2020, at 04:54, hanwenn@gmail.com wrote:
> 
> 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. 

The discussion has turned from (a) passing a required parameter as a reference,
to (b) using a reference as a member variable.  (a) does not imply (b).  You can
pass in a T& and store it in a T* to avoid the constraints that (b) would place
on the use of your class (though they apparently were not previously a problem
in this case).

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

We must be miscommunicating, because I see a lot more than 2.  For some
examples,

    git grep 'vector<[^>]\+> &'

Please clarify.

Thanks,
— 
Dan

[hanwenn, 2020-01-30 22:50:38]

remove pointer

[hanwenn, 2020-01-30 23:22:46]

On 2020/01/30 14:31:07, dan_faithful.be wrote:
> On Jan 30, 2020, at 04:54, mailto:hanwenn@gmail.com wrote:
> > 
> > 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. 
> 
> The discussion has turned from (a) passing a required parameter as a
reference,
> to (b) using a reference as a member variable.  (a) does not imply (b).  You
can
> pass in a T& and store it in a T* to avoid the constraints that (b) would
place
> on the use of your class (though they apparently were not previously a problem
> in this case).
> 
> > The current code overwhelmingly disavows references. The 2 remaining
> > uses (this being one) stand out like a sore thumb.
> 
> We must be miscommunicating, because I see a lot more than 2.  For some
> examples,

There are (were) 2 instance of non-const references passed in .hh files. One
dead method in Score_performer, and Lily_lexer::scan_word

In the lily/ directory

 git grep 'vector<[^>]\+> &' *c|grep -v const

returns 20 results, which is pretty small, given the number of methods in the
code base. A cursory inspection suggests that Mike introduced most of these, and
I would have probably suggested to use pointers there too. I would also change
these signatures if would stumble across them while refactoring something else.
Note that const& are OK as an optimization of call by value; in the caller, the
effect of cont& and call by value is indistinguishable. 

I feel this whole discussion has gone out of hand, and in the interest of
expediency, I have replaced 

  const Input*

with

  Input

in the class declaration, so somebody can give this an LGTM now.

I also have the feeling we spend a lot of time and energy talking past each
other. How about a short voice/video call between the 3 of us to make sure we
are on the same page?

cheers,


> 
>     git grep 'vector<[^>]\+> &'
> 
> Please clarify.
> 
> Thanks,
> — 
> Dan
>

[Dan Eble, 2020-01-31 00:31:34]

On 2020/01/30 23:22:46, hanwenn wrote:
> I feel this whole discussion has gone out of hand, and in the interest of
> expediency, I have replaced 
> 
>   const Input*
> 
> with
> 
>   Input
> 
> in the class declaration, so somebody can give this an LGTM now.

Please read the following with a friendly tone of voice.

I'm having the same trouble I initially had trying to match the description of
this change with its content.  "Renames and reorders functions to clarify the
mechanism. No functional
changes."  Yet in patch set 4, there is a difference other than renaming and
reordering.  Parse_start makes a copy of the Input that it didn't before, and it
also has an additional Input member.  I don't have the background knowledge to
say whether this discrepancy is consequential, just that it doesn't match the
description; but that keeps me from saying "looks good."  Maybe someone else
can.  Either way, I don't think it's too much to ask to expand the description
to reflect the change more accurately.

[hanwenn, 2020-01-31 07:30:23]

On Fri, Jan 31, 2020 at 1:31 AM <nine.fierce.ballads@gmail.com> wrote:
>
> On 2020/01/30 23:22:46, hanwenn wrote:
> > I feel this whole discussion has gone out of hand, and in the interest
> of
> > expediency, I have replaced
> >
> >   const Input*
> >
> > with
> >
> >   Input
> >
> > in the class declaration, so somebody can give this an LGTM now.
>
> Please read the following with a friendly tone of voice.

Thanks for keeping a cool head !

> I'm having the same trouble I initially had trying to match the
> description of this change with its content.  "Renames and reorders
> functions to clarify the mechanism. No functional
> changes."  Yet in patch set 4, there is a difference other than renaming
> and reordering.  Parse_start makes a copy of the Input that it didn't
> before, and it also has an additional Input member.  I don't have the
> background knowledge to say whether this discrepancy is consequential,

It is not.

> just that it doesn't match the description; but that keeps me from
> saying "looks good."  Maybe someone else can.  Either way, I don't think
> it's too much to ask to expand the description to reflect the change
> more accurately.

Locally, I have

    Clean up embedded scheme parsing/evaluation.

    Renames and reorders functions to clarify the mechanism. No
    consequential functional changes.

    Separates input and output parameters.

but I can't find a button to edit the change description.

-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen

[hanwenn, 2020-01-31 07:36:57]

On Fri, Jan 31, 2020 at 8:30 AM Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> Locally, I have
>
>     Clean up embedded scheme parsing/evaluation.
>
>     Renames and reorders functions to clarify the mechanism. No
>     consequential functional changes.
>
>     Separates input and output parameters.
>
> but I can't find a button to edit the change description.

found it. It is in the corner ("edit issue"). It would be nice if
git-cl upload did this automatically.

-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen

[hanwenn, 2020-01-31 07:39:28]

Adapted description.

On Fri, Jan 31, 2020 at 8:36 AM Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
>
> On Fri, Jan 31, 2020 at 8:30 AM Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> > Locally, I have
> >
> >     Clean up embedded scheme parsing/evaluation.
> >
> >     Renames and reorders functions to clarify the mechanism. No
> >     consequential functional changes.
> >
> >     Separates input and output parameters.
> >
> > but I can't find a button to edit the change description.
>
> found it. It is in the corner ("edit issue"). It would be nice if
> git-cl upload did this automatically.
>
> --
> Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen



-- 
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen

[benko.pal, 2020-01-31 10:49:13]

https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc
File lily/parse-scm.cc (right):

https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc#new...
lily/parse-scm.cc:77: const Input *hi = &ps->start_;
I understand (and like) adding the const, but can't understand changing the
reference to pointer even after reading the whole discussion.

[hanwenn, 2020-01-31 10:55:46]

https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc
File lily/parse-scm.cc (right):

https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc#new...
lily/parse-scm.cc:77: const Input *hi = &ps->start_;
On 2020/01/31 10:49:13, benko.pal wrote:
> I understand (and like) adding the const, but can't understand changing the
> reference to pointer even after reading the whole discussion.

the change from & to * here is merely syntactical. 

For me, the discussion around & vs * is about using non-const & in function
signatures and as class members.

However, what I want most at this point is to submit this change, so I can get
on with the next change in the series.

[benko.pal, 2020-01-31 11:04:12]

<hanwenn@gmail.com> ezt írta (időpont: 2020. jan. 31., P, 11:55):
>
>
> https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc
> File lily/parse-scm.cc (right):
>
>
https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc#new...
> lily/parse-scm.cc:77: const Input *hi = &ps->start_;
> On 2020/01/31 10:49:13, benko.pal wrote:
> > I understand (and like) adding the const, but can't understand
> changing the
> > reference to pointer even after reading the whole discussion.
>
> the change from & to * here is merely syntactical.

of course it is; but to me it's nearer to obfuscation than to cleanup.

[Dan Eble, 2020-01-31 17:38:55]

On 2020/01/30 23:22:46, hanwenn wrote:
> In the lily/ directory
> 
>  git grep 'vector<[^>]\+> &' *c|grep -v const
> 
> returns 20 results, which is pretty small, given the number of methods in the
> code base. A cursory inspection suggests that Mike introduced most of these,
and
> I would have probably suggested to use pointers there too. I would also change
> these signatures if would stumble across them while refactoring something
else.

Wouldn't this policy tend toward crimes against readability like the following?

void twiddle_vector(vector<int> *vec)
{
  if (!vec || vec->empty ())
    return;

  for (size_t i = 0; i < vec->size() - 1; ++i)
    {
      if ((*vec)[i] < 10)
         (*vec)[i] = (*vec)[i] + 2 * (*vec)[i + 1];
      else
        (*vec)[i] = (*vec)[i] * 2 - (*vec)[i + 1];
    }
}

How would you approach that?

[hanwenn, 2020-01-31 17:52:45]

On 2020/01/31 17:38:55, Dan Eble wrote:
> On 2020/01/30 23:22:46, hanwenn wrote:
> > In the lily/ directory
> > 
> >  git grep 'vector<[^>]\+> &' *c|grep -v const
> > 
> > returns 20 results, which is pretty small, given the number of methods in
the
> > code base. A cursory inspection suggests that Mike introduced most of these,
> and
> > I would have probably suggested to use pointers there too. I would also
change
> > these signatures if would stumble across them while refactoring something
> else.
> 
> Wouldn't this policy tend toward crimes against readability like the
following?
> 
> void twiddle_vector(vector<int> *vec)
> {
>   if (!vec || vec->empty ())
>     return;
> 
>   for (size_t i = 0; i < vec->size() - 1; ++i)
>     {
>       if ((*vec)[i] < 10)
>          (*vec)[i] = (*vec)[i] + 2 * (*vec)[i + 1];
>       else
>         (*vec)[i] = (*vec)[i] * 2 - (*vec)[i + 1];
>     }
> }
> 
> How would you approach that?

you can do a local alias

  vector<> &v = *vec;

to aid readability.

[Dan Eble, 2020-01-31 18:22:47]

On 2020/01/31 17:52:45, hanwenn wrote:
> you can do a local alias
> 
>   vector<> &v = *vec;
> 
> to aid readability.

The more I think about banning non-const reference parameters, the more I'm
against it.  Google's coding standards may work for them, but their rationale*
for this one is weak.  How can we resolve this disagreement quickly?  Do you
simply have the final say as the project founder?

* "References can be confusing, as they have value syntax but pointer
semantics."

[hanwenn, 2020-01-31 18:32:51]

drop 'hi' local var.

[hanwenn, 2020-01-31 18:33:09]

On 2020/01/31 18:22:47, Dan Eble wrote:
> On 2020/01/31 17:52:45, hanwenn wrote:
> > you can do a local alias
> > 
> >   vector<> &v = *vec;
> > 
> > to aid readability.
> 
> The more I think about banning non-const reference parameters, the more I'm
> against it.  Google's coding standards may work for them, but their rationale*
> for this one is weak.  How can we resolve this disagreement quickly?  Do you
> simply have the final say as the project founder?

Can we have this discussion on a thread separate from this code review?
I want this code to go in.

[dan_faithful.be, 2020-01-31 18:43:34]

On Jan 31, 2020, at 13:33, hanwenn@gmail.com wrote:
> 
> On 2020/01/31 18:22:47, Dan Eble wrote:
>> On 2020/01/31 17:52:45, hanwenn wrote:
>>> you can do a local alias
>>> 
>>>  vector<> &v = *vec;
>>> 
>>> to aid readability.
>> 
>> The more I think about banning non-const reference parameters, the
> more I'm
>> against it.  Google's coding standards may work for them, but their
> rationale*
>> for this one is weak.  How can we resolve this disagreement quickly? 
> Do you
>> simply have the final say as the project founder?
> 
> Can we have this discussion on a thread separate from this code review?
> I want this code to go in.

Well, you can't prevent people from replying to their email, but neither can
their replies prevent you from pushing commits.
— 
Dan

[Carl, 2020-01-31 19:07:05]

On 2020/01/31 18:33:09, hanwenn wrote:
> On 2020/01/31 18:22:47, Dan Eble wrote:
> > On 2020/01/31 17:52:45, hanwenn wrote:
> > > you can do a local alias
> > > 
> > >   vector<> &v = *vec;
> > > 
> > > to aid readability.
> > 
> > The more I think about banning non-const reference parameters, the more I'm
> > against it.  Google's coding standards may work for them, but their
rationale*
> > for this one is weak.  How can we resolve this disagreement quickly?  Do you
> > simply have the final say as the project founder?
> 
> Can we have this discussion on a thread separate from this code review?
> I want this code to go in.

This code is a definite improvement in my book.  I like the names of the
functions, and it seems to me that eliminating the Pars_start class is  a good
idea.

Han-Wen has responded well to comments (even making changes that are not his
preferred way of doing things).

This patch LGTM.

I would like to see some separate discussion about the status of Input and the
use of non-constant reference pointers.  But we shouldn't hold up this patch to
have that discussion.

Carl

[hanwenn, 2020-02-05 08:19:20]

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

[hanwenn, 2020-02-13 11:42:18]

commit 5a4039b700f3a7447401780c720070d14e2891bd
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Jan 31 08:24:44 2020 +0100

    Clean up embedded scheme parsing/evaluation.
    
    Renames and reorders functions to clarify the mechanism. No
    consequential functional changes.
    
    Separates input and output parameters.