Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(19)

Issue 577440044: Use a pointer for the output parameter of Lily_lexer::scan_word (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by hanwenn
Modified:
2 weeks, 2 days ago
Reviewers:
benko.pal, Dan Eble, dan, dak
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : score performer #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -8 lines) Patch
M lily/include/lily-lexer.hh View 1 chunk +1 line, -1 line 0 comments Download
M lily/include/score-performer.hh View 1 1 chunk +0 lines, -1 line 0 comments Download
M lily/lexer.ll View 3 chunks +7 lines, -5 lines 0 comments Download
M lily/parser.yy View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 12
hanwenn
score performer
1 month ago (2020-01-29 06:43:19 UTC) #1
Dan Eble
On 2020/01/29 06:43:19, hanwenn wrote: > score performer Does this relate to the lexer change, ...
4 weeks ago (2020-01-31 12:22:22 UTC) #2
hanwenn
On 2020/01/31 12:22:22, Dan Eble wrote: > On 2020/01/29 06:43:19, hanwenn wrote: > > score ...
4 weeks ago (2020-01-31 18:02:55 UTC) #3
Dan Eble
> Stop using non-const references in function signatures Carrying the discussion over from [1], I ...
4 weeks ago (2020-01-31 19:30:48 UTC) #4
benko.pal
On 2020/01/29 06:43:19, hanwenn wrote: > score performer removed unused: looks good. changing references to ...
3 weeks, 6 days ago (2020-02-01 13:14:43 UTC) #5
Dan Eble
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 ...
3 weeks, 6 days ago (2020-02-01 14:19:49 UTC) #6
dak
nine.fierce.ballads@gmail.com writes: > Consider this function from lily/tie-formatting-problem.cc: > > void > Tie_formatting_problem::score_ties (Ties_configuration *ties) ...
3 weeks, 6 days ago (2020-02-01 14:59:54 UTC) #7
dan_faithful.be
On Feb 1, 2020, at 09:59, David Kastrup <dak@gnu.org> wrote: > > even with a ...
3 weeks, 6 days ago (2020-02-01 16:08:09 UTC) #8
hanwenn
On 2020/01/31 19:30:48, Dan Eble wrote: > > Stop using non-const references in function signatures ...
3 weeks, 6 days ago (2020-02-01 20:10:15 UTC) #9
Dan Eble
On 2020/02/01 20:10:15, hanwenn wrote: > Can I ask that we don't do this on ...
3 weeks, 6 days ago (2020-02-01 20:55:22 UTC) #10
hanwenn
On 2020/02/01 20:55:22, Dan Eble wrote: > On 2020/02/01 20:10:15, hanwenn wrote: > > Can ...
3 weeks, 6 days ago (2020-02-01 21:18:24 UTC) #11
dak
3 weeks, 5 days ago (2020-02-02 17:31:28 UTC) #12
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b