This is an area of LilyPond that I don't understand very well, so feel
free to ignore this question if the answer should be obvious. This
patch removes a call to get_property (stem, "positioning-done") for
cross-staff stems, but doesn't add an alternative anywhere. Is
something else already calli
On 2020/05/09 18:14:40, Valentin Villenave wrote:
> I do, however, worry about the comment before (what used to be) the
> internal_line_count() definition:
>
> // Get the line-count property directly. This is for internal use
when it is
> // known that the line-positions property is not relevant.
LGTM. Thanks.
https://codereview.appspot.com/561810043/
https://codereview.appspot.com/554030043/diff/583870043/lily/note-head.cc
File lily/note-head.cc (right):
https://codereview.appspot.com/554030043/diff/583870043/lily/note-head.cc#newcode114
lily/note-head.cc:114: if (stem and ! ly_scm2bool (get_property (stem,
"cross-staff")))
Please use && and
https://codereview.appspot.com/576090043/diff/569740045/lily/staff-symbol.cc
File lily/staff-symbol.cc (right):
https://codereview.appspot.com/576090043/diff/569740045/lily/staff-symbol.cc#newcode116
lily/staff-symbol.cc:116: return ly_scm2floatvector (line_positions);
The suggested patch to exp
https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):
https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc#newcode268
lily/multi-measure-rest.cc:268: bool oneline = (!staff) ||
Staff_symbol::line_
https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):
https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc#newcode268
lily/multi-measure-rest.cc:268: bool oneline = (!staff) ||
Staff_symbol::line_
https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):
https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc#newcode268
lily/multi-measure-rest.cc:268: bool oneline = (!staff) ||
Staff_symbol::line_
I prefer patch set 2.
https://codereview.appspot.com/577840053/diff/561760044/lily/lexer.ll
File lily/lexer.ll (right):
https://codereview.appspot.com/577840053/diff/561760044/lily/lexer.ll#newcode928
lily/lexer.ll:928: // use more SCM for this.
Is this what you've just done, or is there still m
On 2020/04/27 22:07:07, dak wrote:
>
https://codereview.appspot.com/577840053/diff/561760044/lily/lily-lexer.cc
> File lily/lily-lexer.cc (right):
>
>
https://codereview.appspot.com/577840053/diff/561760044/lily/lily-lexer.cc#newcode97
> lily/lily-lexer.cc:97: Protected_scm Lily_lexer::keytable_;
https://codereview.appspot.com/561680043/diff/577820044/lily/include/transform.hh
File lily/include/transform.hh (right):
https://codereview.appspot.com/561680043/diff/577820044/lily/include/transform.hh#newcode33
lily/include/transform.hh:33: int print_smob (SCM p, scm_print_state *)
const;
Thi
I see I'm late to the party with this comment, but what are the error
bars on that 0.29%? I'm not sure the decreased readability is worth it.
https://codereview.appspot.com/551730043/
On 2020/04/22 07:43:17, hanwenn wrote:
> Finally, to Dan's point: I haven't looked at heap profiling.
...
> For now, I don't intend to try this out
I was asking for David's benefit. Personally, this change doesn't
concern me. In my professional work, we usually avoid lists for some of
the reason
On 2020/04/21 23:39:55, dak wrote:
> influence on algorithmic complexity. In return for better memory
locality you
> buy into quite larger memory fragmentation, and we have scores of
comparatively
> modest size already exhausting memory. All that exhausted memory
needs to get
Han-Wen, do you hav
https://codereview.appspot.com/547980044/diff/548010043/lily/skyline.cc
File lily/skyline.cc (right):
https://codereview.appspot.com/547980044/diff/548010043/lily/skyline.cc#newcode348
lily/skyline.cc:348: result.push_back (buildings->at (0));
at() involves a boundary check that is not necessary
On 2020/04/20 19:36:39, dak wrote:
> Polish and extend tools in callback.hh
LGTM.
I have a hunch that the MFPn_WRAP macros could be turned into something
more typical of C++, but I don't blame you for resting at this point.
https://codereview.appspot.com/551780043/
On 2020/04/18 18:14:33, thomasmorley651 wrote:
I ran make check and didn't see any differences. Without test coverage,
someone could easily break this again without knowing it.
https://codereview.appspot.com/553930044/
On 2020/04/18 17:53:34, dak wrote:
> Well, after a refactoring (patch set 5) it was just a few lines (patch
set 6)
> but unfortunately this time definitely requiring some hand-spun type
trait
> punning. Not particularly great.
Not bad, either. LGTM.
https://codereview.appspot.com/549890043/
On 2020/04/18 14:31:33, dak wrote:
> If we consider type_traits as a sunk cost, I'll see whether I can find
anything
> in it to address the inheritance wart you complained of.
As you already put in some time there, I wouldn't think less of you if
you just left the static_cast in Global_context and
LGTM but there are a couple of things I strongly encourage considering.
https://codereview.appspot.com/549890043/diff/573710043/lily/global-context.cc
File lily/global-context.cc (right):
https://codereview.appspot.com/549890043/diff/573710043/lily/global-context.cc#newcode49
lily/global-context
https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc
File lily/skyline.cc (right):
https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode75
lily/skyline.cc:75: i.print ();
I'm sorry, I gave you imperfect advice here because I had an iterator on
my mind
Reviewers: lemzwerg,
Message:
On 2020/04/18 05:53:25, lemzwerg wrote:
>
https://codereview.appspot.com/567450043/diff/555680043/lily/performance-scheme.cc#oldcode31
> lily/performance-scheme.cc:31: LY_DEFINE (ly_performance_set_header_x,
> "ly:performance-set-header!",
> Will this removal need a `
On 2020/04/17 19:07:44, hanwenn wrote:
> The profile data itself is also a more precise source for measuring
> improvements that are close to the noise level, like
> https://codereview.appspot.com/551730043/
Investigating improvements that are close to the noise level--never
mind, I won't tell you
On 2020/04/17 17:17:27, lemzwerg wrote:
>
https://codereview.appspot.com/559870043/diff/553910043/lily/include/box.hh#newcode38
> lily/include/box.hh:38: /// smallest box enclosing `this` and `b`
> Why '///' instead of '//'?
It's a habit some people have picked up from various experiences:
https:/
You're not doing this for performance, just for simplicity, correct?
https://codereview.appspot.com/565920043/
LGTM
https://codereview.appspot.com/571980043/diff/581920043/lily/skyline.cc
File lily/skyline.cc (right):
https://codereview.appspot.com/571980043/diff/581920043/lily/skyline.cc#newcode164
lily/skyline.cc:164: if (ret >= x_[LEFT] && ret <= x_[RIGHT] &&
!std::isinf (ret))
Is this x_.contains (re
The idea makes sense to me. I haven't reviewed the whole change.
https://codereview.appspot.com/571980043/diff/579590043/lily/skyline.cc
File lily/skyline.cc (right):
https://codereview.appspot.com/571980043/diff/579590043/lily/skyline.cc#newcode100
lily/skyline.cc:100: x_[START] = start;
Initi
This change per se LGTM. I remember discussing this syntactic change
briefly on the list a few(?) months ago, so this is not surprising.
I'm quite pleased with this change, actually. I remember how I felt the
first time I came across klass->a_macro_actually(...) and couldn't find
the method in k
On 2020/04/13 17:22:52, hanwenn wrote:
> > If you're not interested in doing this, I might try it myself.
>
> By structuring it like this, you enforce the implementation to store
the
> key/value as SCM cells, which is exactly what we want to get away
from.
OK, I didn't understand that from the d
https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-properties.hh
File lily/include/mutable-properties.hh (right):
https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-properties.hh#newcode31
lily/include/mutable-properties.hh:31: class Iterator {
LGTM
https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc
File flower/rational.cc (right):
https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc#newcode257
flower/rational.cc:257: if (is_infinity ())
The hazard of reviewing this class is that there's al
On 2020/04/04 07:53:24, hahnjo wrote:
> 1: As a side note, this means there's a very funny workaround: Just
push *any*
> commit to master with a pure ASCII description and you'll not see the
failure
> anymore :D
In the distance, a brass band plays "The Star-Spangled Banner."
https://codereview.a
> Remove references to test-output-distance.ly.
>
> It was removed in commit 8a74a0e ("Remove
> input/regression/test-output-distance.ly")
I must have slept through that. Had I been paying attention, I would
have pointed out what the documentation you propose to remove says:
This test change
On 2020/03/27 23:16:40, Dan Eble wrote:
> Is the impact (if any) on existing scores important? (cases that we
might not
> have in regression tests but that might irk users?)
... and speaking of regression tests, if you don't want someone to break
your work later, it would be a good idea to add on
Is the impact (if any) on existing scores important? (cases that we
might not have in regression tests but that might irk users?)
https://codereview.appspot.com/553760043/
On 2020/03/26 11:35:11, dak wrote:
> On 2020/03/26 01:25:57, Dan Eble wrote:
> > It's worth emphasizing this: even with these improvements to the
clang-format
> > configuration, those who use clang-format will introduce a truckload
of
> > differences from the traditional indentation.
...
> correctl
Reviewers: ,
Message:
It's worth emphasizing this: even with these improvements to the
clang-format configuration, those who use clang-format will introduce a
truckload of differences from the traditional indentation.
Description:
https://sourceforge.net/p/testlilyissues/issues/5862/
Prefer asty
Jonas,
I'm low on energy, so I'm not going to review this now, but I there are
a few points I want to mention.
1. thanks for cleaning things up!
2. maybe the rounding change should wait for 2.21.1
3. please don't look at real.hh/cc; I've got a patch for it that I'm
waiting to post
https://coder
On 2020/03/11 00:34:32, davidsg wrote:
> hyphen-engraver now also listens for transition-events, and makes
transitions or
> hyphens based on event class.
I know some people hate talking about names, but can we talk about this
one? Think of the kinds of events that a "transition event" might
prope
https://codereview.appspot.com/553620043/diff/581760043/scripts/musicxml2ly.py
File scripts/musicxml2ly.py (right):
https://codereview.appspot.com/553620043/diff/581760043/scripts/musicxml2ly.py#newcode2287
scripts/musicxml2ly.py:2287: if n._measure_position == Rational(0) and n
!= voice._elemen
On 2020/03/07 21:36:04, hahnjo wrote:
> On 2020/03/07 21:24:48, hanwenn wrote:
> > I don't care much what we call this, and we can certainly change
this, but
> > it's historically been called check.
>
> Yes, but 'check' for LilyPond also does the comparison to
test-baseline. As
> 'check' depends o
On 2020/03/07 18:59:54, hahnjo wrote:
> I think this is wrong. Instead 'make test' should recurse into the
> subdirectories.
Agreed. That would be here in the top-level GNUMakefile.in:
test: test-pre
@echo 'To begin investigating regression-test crashes, use'
@echo
@echo
On 2020/03/07 18:55:02, hanwenn wrote:
> > On 2020/03/07 18:40:57, Dan Eble wrote:
> > > Don't you think these two should be combined? I don't see value
in leaving
> > > the first commit with an incorrect target name.
> >
> > I'll send this separately. It has nothing to do with
output-distance.
The logging and diffing changes basically LGTM (see my questions). I
paid no attention to other changes (self-test, stencil comparison,
python idioms, etc.).
https://codereview.appspot.com/581770043/diff/571840081/scripts/build/output-distance.py
File scripts/build/output-distance.py (left):
ht
On 2020/03/07 17:49:55, hanwenn wrote:
> it's a sequence of 6 commmits, which you can inspect here:
>
> https://github.com/hanwen/lilypond/pull/2
The fact that the HTML table now includes rows for removed files is
worth mentioning in this change.
commit 613d8478797784ecc9e7676639375e7f18d4abf
On 2020/03/07 17:49:55, hanwenn wrote:
> it's a sequence of 6 commmits, which you can inspect here:
>
> https://github.com/hanwen/lilypond/pull/2
Don't you think these two should be combined? I don't see value in
leaving the first commit with an incorrect target name.
commit 37c6ba676f4722a
The title of the review is "5824 Fix test target name in
python/GNUmakefile" but the title of ticket 5824 is "output-distance
refactor". It's hard to tell what you want us to review. Everything?
Just the makefiles? Just output-distance? Please update the title and
description in both places to
On 2020/03/06 16:28:01, hanwenn wrote:
>
>
https://codereview.appspot.com/581770043/diff/561510043/scripts/build/output-distance.py
>
> it leaves striped cells for removed files. I don't think it's feasible
to do
> much better, or we'd have to extend output-distance to infer what kind
of files
>
On 2020/02/07 12:34:38, dak wrote:
But it has
> evaded me to find a way of expressing "end cadenza and bar". The best
I could
> do so far could be expressed as
>
> \cadenzaOffAfter =
> #(define-music-function (last-note) (ly:music?)
>#{ \partial #(ly:music-duration 1 0 (ly:moment-main
(ly:mus
On 2020/03/05 10:33:57, hanwenn wrote:
> > the script fail on None.strip(), though not the best option, is
still better
> > than treating this as discreetly as the intentional removal of a
test case.
>
> I disagree. Having the script crash prevents us from forming a
complete
> impression of what a
https://codereview.appspot.com/583590043/diff/571790043/scripts/build/output-distance.py
File scripts/build/output-distance.py (right):
https://codereview.appspot.com/583590043/diff/571790043/scripts/build/output-distance.py#newcode531
scripts/build/output-distance.py:531: if self.contents[0] is
On 2020/02/28 23:31:17, Dan Eble wrote:
> This does not look good. I expect this change to break the feature I
added in
> 0d72930e579a5784ecb26da2f9880d8c9da05e71 (Issue 5635). Well, a call to
strip
> (None) is possibly evidence that someone already broke it, but let's
at least
> avoid making it w
How does this interact with CPU_COUNT, which is documented in the
Contributor's Guide thus (emphasis mine)?
The most time consuming task for building the documentation is
running LilyPond to build images of music, and *** there cannot be
several simultaneously running lilypond-book ins
LGTM
https://codereview.appspot.com/559500043/diff/559510043/lily/grob.cc
File lily/grob.cc (right):
https://codereview.appspot.com/559500043/diff/559510043/lily/grob.cc#newcode733
lily/grob.cc:733: while (unsmob (cause))
I appreciate that issuing warnings is not performance-sensitive, and
that
On 2020/02/18 10:20:31, michael.kaeppler wrote:
> This patch has already been counted down, however, I would
> like to have at least one LGTM to the last version of this patch
before pushing
> this.
> Knowing that this is only an intermediate state and further
clarification would
> help.
I skimme
On 2020/02/17 18:41:58, dak wrote:
> Are you sure this is actually a working idea? At the beginning of
music, Score
> does not exist and 'Timing is only (reliably?) established as an alias
by the
> Timing_translator. For polyrhythmic pieces, the Timing context alias
is moved
> down the hierarchy
Reviewers: ,
https://codereview.appspot.com/557440043/diff/547670043/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):
https://codereview.appspot.com/557440043/diff/547670043/ly/music-functions-init.ly#newcode1319
ly/music-functions-init.ly:1319: 'origin (*location*)
I'm curiou
https://codereview.appspot.com/583510045/diff/559490043/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):
https://codereview.appspot.com/583510045/diff/559490043/lily/axis-group-interface.cc#newcode277
lily/axis-group-interface.cc:277: Interval_t rank_span
(rank_span_int[LE
LGTM. Changing to std::string is a definite improvement. Thanks.
https://codereview.appspot.com/579310043/diff/557420043/lily/engraver.cc
File lily/engraver.cc (right):
https://codereview.appspot.com/579310043/diff/557420043/lily/engraver.cc#newcode121
lily/engraver.cc:121: if (!scm_is_pair (p
LGTM
https://codereview.appspot.com/571610043/
https://codereview.appspot.com/579310043/diff/549550043/lily/include/source-file.hh
File lily/include/source-file.hh (right):
https://codereview.appspot.com/579310043/diff/549550043/lily/include/source-file.hh#newcode44
lily/include/source-file.hh:44: /* The input data, plus an extra \0 to
termi
https://codereview.appspot.com/561390043/diff/567180043/lily/include/smobs.hh
File lily/include/smobs.hh (right):
https://codereview.appspot.com/561390043/diff/567180043/lily/include/smobs.hh#newcode312
lily/include/smobs.hh:312: static size_t count;
It seems that this is initialized to zero bec
On 2020/02/07 09:19:03, hanwenn wrote:
> On 2020/02/06 14:29:55, Dan Eble wrote:
> More code means more maintenance liability, so unless
> it either solves a problem, or it simplifies the existing system, it
would be a
> net negative.
You're preaching to the choir.
> I would really like the prob
On 2020/02/07 11:27:56, hanwenn wrote:
> (why is this review closed?)
Jonas reviewed the results and I pushed the change and closed the
review.
https://codereview.appspot.com/559460043/
On 2020/02/06 09:59:26, hanwenn wrote:
> can you add a pointer to the commit that introduces the previous code?
I'm in
> the middle of a laptop switch , so I can't look it up right now.
I've added this link to the description of the current review.
https://codereview.appspot.com/579250043/diff/565
Reviewers: thomasmorley651, hanwenn,
Message:
The reviewers are turning the first question I asked around and asking
it back to me. I don't know if this is useful without other stuff I've
been working on. That's why I've posted it for review. I thought that
you (well, mainly I thought that Davi
Reviewers: ,
Message:
Regtest differences attached to the ticket are expected. I would
appreciate independent confirmation; is anyone interested in taking a
little time to understand the case? After that, I think it will make
sense to push this, since the code was reviewed last week and is just
e
On 2020/02/05 17:52:20, dak wrote:
> configure.ac:367: STEPMAKE_PROGS(CLANG_FORMAT, clang-format-9
clang-format,
> OPTIONAL, 9, 9)
> Interesting. This gives a warning when it isn't installed? I think
that we
> don't usually flag dependencies of components not involved in either
building or
> runn
On 2020/02/05 18:17:25, c_sorensen wrote:
> I recognize that Mike Solomon has a different opinion. I mean no
disrespect to
> Mike, Janek, Han-Wen, or any other member of the LilyPond team. I
highly value
> the team spirit of the LilyPond team.
Well said. Here's the current tally as I understand
Reviewers: ,
Description:
https://sourceforge.net/p/testlilyissues/issues/5739/
These are not yet intended for routine use by contributors. They are
meant to help explore the differences between astyle and clang-format.
Please review this at https://codereview.appspot.com/565620043/
Affected f
https://codereview.appspot.com/579280043/diff/579290043/Documentation/notation/rhythms.itely
File Documentation/notation/rhythms.itely (right):
https://codereview.appspot.com/579280043/diff/579290043/Documentation/notation/rhythms.itely#newcode3325
Documentation/notation/rhythms.itely:3325: bar
On 2020/02/05 09:03:22, hahnjo wrote:
> In my opinion we should have a thread on lilypond-devel. In particular
it should
> lay out what the motivation is / which problem is to be solved. (see
questions
> by David and Karlin)
Other questions:
You're asking for agreement from "the whole LilyPond co
On 2020/02/04 23:40:03, dak wrote:
> Personally, the best of two worlds would be if a ping-pong between our
current
> Astyle and clang-format would end up stable after one application of
each. That
> way, putting the code base through both Astyle and clang-format
semi-regularly
> would put it into
I'm running some of my patches through clang-format as I prepare to push
them.
This is an example of a kind of change it wants to make:
- const array key {column_rank, dir};
+ const array key{column_rank, dir};
Note the space after key. The setting that controls this is
SpaceBeforeCpp11Braced
Reviewers: lemzwerg,
Message:
On 2020/02/04 10:45:58, lemzwerg wrote:
> lily/accidental-placement.cc:62: scm_from_long (stagger ?
context_hash : 1));
> Are you actually trying clang-format? There's one space too much :-)
I was too lazy to deal with Han-Wen's config file as a patch. Now that
it
Reviewers: hanwenn,
Message:
In the review for the first attempt, David wrote:
> It's probably more an academical remark, but a "kosher" way of doing
that might
> be using scm_to_int (scm_length (...)) instead of scm_ilength (...).
etc.
I'll make this change because setting a good example is valu
The changes to lily/break-substitution.cc are exactly what I would do.
What do you think about splitting those into their own commit and
pushing them?
https://codereview.appspot.com/557190043/
Thanks for the reviews, g
https://codereview.appspot.com/573500043/diff/561420043/lily/beam-quanting.cc
File lily/beam-quanting.cc (right):
https://codereview.appspot.com/573500043/diff/561420043/lily/beam-quanting.cc#newcode1049
lily/beam-quanting.cc:1049: configs.clear ();
On 2020/02/03 17:57:
Reviewers: dak, hahnjo,
Message:
On 2020/02/03 18:01:09, dak wrote:
> Stupid question: unique_ptr has the purpose of deallocating memory
when the last
> reference is gone.
When the single, owning reference is gone. It's not reference counted.
> But we have an entire Scheme allocation system exa
LGTM
https://codereview.appspot.com/547560044/diff/555240043/lily/spring.cc
File lily/spring.cc (right):
https://codereview.appspot.com/547560044/diff/555240043/lily/spring.cc#newcode119
lily/spring.cc:119: avg_stretch /= static_cast (springs.size ());
This is not an issue with this change, but
LGTM
https://codereview.appspot.com/563460043/diff/557300058/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):
https://codereview.appspot.com/563460043/diff/557300058/lily/page-layout-problem.cc#newcode739
lily/page-layout-problem.cc:739: = overflow / static_cast
(space_count
On 2020/02/02 15:33:05, hahnjo wrote:
> > > I'd probably have chosen txt.
> >
> > Ok, will do.
>
> Meh: I did the change and it works, but subprocess_system in
python/lilylib.py
> will still output the .ly name. As this file doesn't exist anymore, it
will just
> make investigations in case of an
On 2020/02/02 09:52:37, hahnjo wrote:
>
https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book.py#newcode432
> scripts/lilypond-book.py:432: snippet_names_file =
'snippet-names-%s.ly' %
> checksum
> On 2020/02/01 16:54:40, Dan Eble wrote:
> > It's strange that this is named *
I approve of your use of braces for even single-statement blocks.
https://codereview.appspot.com/581580043/diff/561400052/lily/pitch.cc
File lily/pitch.cc (right):
https://codereview.appspot.com/581580043/diff/561400052/lily/pitch.cc#newcode162
lily/pitch.cc:162: if (qt < int (sizeof (accname) /
LGTM
https://codereview.appspot.com/577440043/
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 p
On 2020/02/01 12:06:02, Dan Eble wrote:
> I see two uses of strdup.
> The one in piano-pedal-engraver looks like it could be eliminated by
using
> std::string instead of a C string.
See https://codereview.appspot.com/557270049/ .
It seems worthwhile independent of the gnu++11 question.
https://co
On 2020/02/01 15:04:41, trueroad wrote:
> In `-std=c++11`, most POSIX functions/definitions cannot be used.
> Also `putenv ()` and `chroot ()` cannot be used.
But is it the case that the implementations of those functions requires
GNU C++ extensions, or is it the case that their header files are n
LGTM, but I'm no guru.
https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book.py
File scripts/lilypond-book.py (right):
https://codereview.appspot.com/555220043/diff/553480046/scripts/lilypond-book.py#newcode432
scripts/lilypond-book.py:432: snippet_names_file = 'snippet-n
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;
On 2020/02/01 12:06:02, Dan Eble wrote:
> I see two uses of strdup.
> The one in piano-pedal-engraver looks like it could be eliminated by
using
> std::string instead of a C string.
> The one in relocate.cc looks like it could be replaced by
strlen+malloc+strncpy.
I can work on a patch for these.
On 2020/02/01 08:04:36, trueroad wrote:
> Even if we define `M_PI`, an error raises since there is no `strdup
()`.
I see two uses of strdup.
The one in piano-pedal-engraver looks like it could be eliminated by
using std::string instead of a C string.
The one in relocate.cc looks like it could be r
Preliminary reaction: I don't like it. This might be a case of missing
includes which are implicitly included in one case and not in another.
https://codereview.appspot.com/579270051/
https://codereview.appspot.com/581580043/diff/557260048/lily/pitch.cc
File lily/pitch.cc (right):
https://codereview.appspot.com/581580043/diff/557260048/lily/pitch.cc#newcode162
lily/pitch.cc:162: if (qt >= 0 && qt < int (sizeof(accname) /
sizeof(const char*))) {
Instead of casting to int and c
https://codereview.appspot.com/561390043/diff/557260051/lily/include/score-engraver.hh
File lily/include/score-engraver.hh (right):
https://codereview.appspot.com/561390043/diff/557260051/lily/include/score-engraver.hh#newcode33
lily/include/score-engraver.hh:33: GC_word last_gc_count_;
FYI: Bec
> 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
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. Ho
On 2020/01/31 18:01:32, hanwenn wrote:
> > The second underscore makes this stand out from the other functions
in this
> > group.
>
> it does. Do you have a suggestion to improve?
ly_scm2utf8string? ly_scm2utf8?
https://codereview.appspot.com/577440043/
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 pro
On 2020/01/31 09:39:33, hanwenn wrote:
> I've applied your suggestion; PTAL.
If Werner likes it, I'm fine with it. I haven't tried it myself because
I want to avoid being drawn into discussing the details of the style,
but I like seeing movement toward a better process.
https://codereview.appsp
1 - 100 of 506 matches
Mail list logo
