Use a pointer for the output parameter of Lily_lexer::scan_word

DO NOT SUBMIT 

Stop using non-const references in function signatures

* Use a pointer for the output parameter of Lily_lexer::scan_word

* Remove unused declaration in Score_performer.

[hanwenn, 2020-01-29 06:43:19]

score performer

[Dan Eble, 2020-01-31 12:22:22]

On 2020/01/29 06:43:19, hanwenn wrote:
> score performer

Does this relate to the lexer change, or does it belong on its own?

[hanwenn, 2020-01-31 18:02:55]

On 2020/01/31 12:22:22, Dan Eble wrote:
> On 2020/01/29 06:43:19, hanwenn wrote:
> > score performer
> 
> Does this relate to the lexer change, or does it belong on its own?

changed description.

[Dan Eble, 2020-01-31 19:30:48]

> Stop using non-const references in function signatures

Carrying the discussion over from [1], I would like to hear a clear decision
that this is the way LilyPond is going to be coded--something more definite than
one person proposing a change and another saying it looks good.  If this is the
way things are going to be, contributors and reviewers would also benefit from
guidance on when a function should validate the pointers it receives, and if it
doesn't, how that ought to be documented to make up for not being allowed to
pass by reference.

[1] https://codereview.appspot.com/577410045/

[benko.pal, 2020-02-01 13:14:43]

On 2020/01/29 06:43:19, hanwenn wrote:
> score performer

removed unused: looks good.

changing references to pointers: looks BAAAAD.

[Dan Eble, 2020-02-01 14:19:49]

Consider this function from lily/tie-formatting-problem.cc:

    void
    Tie_formatting_problem::score_ties (Ties_configuration *ties) const
    {
      if (ties->scored_)
        return;
    
      score_ties_configuration (ties);
      score_ties_aptitude (ties);
      ties->scored_ = true;
    }

What is the benefit to using pointers here?  I expect a supporter of Google's
style to answer that it's true that there is no benefit here, but there is a
benefit somewhere up the call chain.  That's here:

    variant.reset_score ();
    score_ties (&variant);

    if (variant.score () < best.score ())
      {
        best = variant;
      }

That is difficult to misunderstand with or without the ampersand.  The non-null
information that references communicate is more useful than what the ampersand
provides here.

[dak, 2020-02-01 14:59:54]

nine.fierce.ballads@gmail.com writes:

> Consider this function from lily/tie-formatting-problem.cc:
>
>     void
>     Tie_formatting_problem::score_ties (Ties_configuration *ties) const
>     {
>       if (ties->scored_)
>         return;
>     
>       score_ties_configuration (ties);
>       score_ties_aptitude (ties);
>       ties->scored_ = true;
>     }
>
> What is the benefit to using pointers here?  I expect a supporter of
> Google's style to answer that it's true that there is no benefit here,
> but there is a benefit somewhere up the call chain.  That's here:
>
>     variant.reset_score ();
>     score_ties (&variant);
>
>     if (variant.score () < best.score ())
>       {
>         best = variant;
>       }
>
> That is difficult to misunderstand with or without the ampersand.

Well, it is not clear whether this is just an input parameter or in-out,
and how efficiently it is being passed.  The efficiency arguably is only
the business of the called site, not every caller, and it is still not
possible to see the difference between Ties_configuration *ties and
Ties_configuration const *ties at the call site.

> The non-null information that references communicate is more useful
> than what the ampersand provides here.

Well, if we really wanted to signify the parameter as being used in-out,
we could do

score_ties (*&variant);

even with a reference parameter.  Of course this does not "really" give
an indication about this being in-out, but neither does &variant.

If it's important, one could use

score_ties (/*in-out*/ variant);

> https://codereview.appspot.com/577440044/

I think this convention is supposed to make it easy to see which
parameters may be output parameters.  But that makes a whole lot more
sense as a conventions for types like int (where passing by pointer
would be unusual) rather than class types which you basically always
want to pass by using a memory reference, whether in the form of a C++
reference or a pointer, regardless of whether they are input-only or
in-out or output-only.

-- 
David Kastrup

[dan_faithful.be, 2020-02-01 16:08:09]

On Feb 1, 2020, at 09:59, David Kastrup <dak@gnu.org> wrote:
> 
> even with a reference parameter.  Of course this does not "really" give
> an indication about this being in-out, but neither does &variant.

That's an important point.  What indicates that a parameter is _not_ an
out-parameter is "const".  You might happen to have that near enough to the call
site to be helpful.

    const Foo& foo = ...;
    int mp = calc_midpoint (foo);

Where you don't have that, and you want to leave no doubt that a function does
not modify your object, you can declare a separate const reference and pass
that, or you can use a convenience function like std::as_const from C++17,

    Foo foo;
    string s = to_string (ly::as_const (foo));
    ...
    return calc_midpoint (foo); // might modify foo

I hope that that example makes as_const and "might modify foo" look like the
result of an overabundance of caution.  I don't mean to suggest that as_const or
comments should be dictated in situations like this anymore than I think that
passing out-parameters as pointers should be dictated.
--
Dan

[hanwenn, 2020-02-01 20:10:15]

On 2020/01/31 19:30:48, Dan Eble wrote:
> > Stop using non-const references in function signatures
> 
> Carrying the discussion over from [1], I would like to hear a clear decision

Can I ask that we don't do this on a code review, but in a separate thread?

[Dan Eble, 2020-02-01 20:55:22]

On 2020/02/01 20:10:15, hanwenn wrote:
> Can I ask that we don't do this on a code review, but in a separate thread?

Why?  The review is where the patch meister (James) looks when he decides
whether to advance the patch through the countdown.  If there were no activity
here, he might advance the patch to push, not recognizing that it is
controversial.

[hanwenn, 2020-02-01 21:18:24]

On 2020/02/01 20:55:22, Dan Eble wrote:
> On 2020/02/01 20:10:15, hanwenn wrote:
> > Can I ask that we don't do this on a code review, but in a separate thread?
> 
> Why?  The review is where the patch meister (James) looks when he decides
> whether to advance the patch through the countdown.  If there were no activity
> here, he might advance the patch to push, not recognizing that it is
> controversial.

I marked it DO NOT SUBMIT.

We should discuss it separately, because likely the discussion will touch on an
wider
issue of C++ style and future plans for the code base, and doesn't pertain to
this small
change of the code.

[dak, 2020-02-02 17:31:28]

nine.fierce.ballads@gmail.com writes:

>> Stop using non-const references in function signatures
>
> Carrying the discussion over from [1], I would like to hear a clear
> decision that this is the way LilyPond is going to be coded--something
> more definite than one person proposing a change and another saying it
> looks good.  If this is the way things are going to be, contributors and
> reviewers would also benefit from guidance on when a function should
> validate the pointers it receives, and if it doesn't, how that ought to
> be documented to make up for not being allowed to pass by reference.
>
> [1] https://codereview.appspot.com/577410045/
>
>
> https://codereview.appspot.com/577440044/

My own definite rule here would be:

Don't pass objects with a Scheme-determined lifetime by reference.  Use
pointers instead.  "Scheme-determined" may be replaced with "dynamic
lifetime" for general C++ code, but I don't think we really have all
that much new/delete outside of SCM handling.  But for example an
additional output parameter of SCM type itself in my book already
warrants passing by reference.  Which would include this particular
issue.

I would not have chosen this way if I had not thought it the most
appropriate.  It's actually a fairly recent commit, namely

commit cc9f25fa7ad9eecd1d1d9cfc2b3c50d96a847386
Author: David Kastrup <dak@gnu.org>
Date:   Sat Apr 1 14:54:05 2017 +0200

    Issue 5113/1: Reorganize Lily_lexer::scan_bare_word
    
    This also redefines LilyPond's manners of converting simple
    expressions into music, most notably checking drum types for being
    defined before accepting them as note values.

We don't actually have a lot of functions having to, in essence, return
more than a single value so actually _either_ passing a pointer or
reference for that purpose is quite rare in the code base.  So I would
not call this use of a reference exceptional: we don't use pointers for
that purpose a lot either.

-- 
David Kastrup