Hello,
Here is the current patch countdown list. The next countdown will be on
April 15th
A quick synopsis of all patches currently in the review process can be
found here:
http://philholmes.net/lilypond/allura/
***
Push:
5884 Clean up many unused global variables - Jonas Hahnfel
On Mon, Apr 13, 2020 at 10:14 AM wrote:
>
> Hello,
>
> Here is the current patch countdown list. The next countdown will be on
> April 15th
>
> David Kastrup
> https://sourceforge.net/p/testlilyissues/issues/5882
> http://codereview.appspot.com/573670043
I want to register my objection to this
Waiting ... probably not. For discussion I usually leave it on or put
back to 'review' - as long as the patch passes the tests, it can sit at
review for however long it takes. I check each countdown. If nothing
gets resolved it can get set back to 'needs_work'.
Waiting implies that the patch i
Reviewers: ,
Message:
Contains two commits:
[PATCH 2/2] Revert "Load only the default font for
System_start_delimiter"
[PATCH 1/2] Regtest for setting SystemStartGrob.style to 'brace
Description:
Revert "Load only the default font for System_start_delimiter"
This reverts commit 430bad24a2d15ec66
https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly
File input/regression/system-start-brace-style.ly (right):
https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly#newcode26
input/regression/system-s
some nits
https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly
File input/regression/system-start-brace-style.ly (right):
https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly#newcode4
input/regression
David Kastrup writes:
> Han-Wen Nienhuys writes:
>
>> On Tue, Feb 11, 2020 at 10:17 PM David Kastrup wrote:
>>
>>> > the reason being that it is better if the source code looks like plain
>>> C++,
>>> > even though they might actually be macros that do advanced magic. Having
>>> > normal lookin
Hi David,
Although you have changed the subject to "Resolving standoffs" your
email reads like an attempt to force a resolution - in your favour -
of one *particular* standoff. It seems to be more of a protest than an
attempt to elicit discussion.
I am both a lurker and not capable of understandi
If the function is not meant to be implemented / used, it should be
explicitly deleted (C++11).
https://codereview.appspot.com/557680043/
>From visual inspection, LGTM.
I only wonder whether we should use
if
{
foo(...);
}
or
if
foo(...);
for single statements – right now, the source code contains both
variants...
https://codereview.appspot.com/561640043/
https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc
File lily/grob-scheme.cc (left):
https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc#oldcode326
lily/grob-scheme.cc:326:
This should very probably not be part of this patch?
https://codereview.a
Reviewers: lemzwerg,
Message:
On 2020/04/13 14:58:01, lemzwerg wrote:
> From visual inspection, LGTM.
>
> I only wonder whether we should use
>
> if
> {
> foo(...);
> }
>
> or
>
> if
> foo(...);
>
> for single statements – right now, the source code contains both
variants...
https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc
File lily/grob-scheme.cc (left):
https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc#oldcode326
lily/grob-scheme.cc:326:
On 2020/04/13 14:56:45, hahnjo wrote:
> This should very probably not be p
A couple of counter points:
100% of the reviewers of the change you proposed disagreed with it.
You call that a veto, but I don't think you can call that consensus
either. Maybe you mean that you got to push changes in the past
because nobody else could understand what they did?
I have tried to d
On 2020/04/13 14:55:12, hahnjo wrote:
> If the function is not meant to be implemented / used, it should be
explicitly
> deleted (C++11).
Done.
https://codereview.appspot.com/557680043/
LGTM
https://codereview.appspot.com/557680043/
Kevin Barry writes:
> Hi David,
>
> Although you have changed the subject to "Resolving standoffs" your
> email reads like an attempt to force a resolution - in your favour -
> of one *particular* standoff.
Yes and no. It is foreseeable that with an influx of developers into
LilyPond, there wil
Han-Wen Nienhuys writes:
> Finally you claim that my incompetence ("[Han-Wen] would not
> be able") is a factor in this discussion. I obviously disagree with
> this assessment. I'll let https://codereview.appspot.com/575990043/
> speak for itself.
I would not call it incompetence if realising th
> On 13 Apr 2020, at 17:15, hanw...@gmail.com wrote:
>
> On 2020/04/13 14:55:12, hahnjo wrote:
>> If the function is not meant to be implemented / used, it should be
> explicitly
>> deleted (C++11).
>
> Done.
>
> https://codereview.appspot.com/557680043/
One might prefer having a deleted oper
On 2020/04/13 15:06:16, hanwenn wrote:
>
https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc
> File lily/grob-scheme.cc (left):
>
>
https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc#oldcode326
> lily/grob-scheme.cc:326:
> On 2020/04/13 14:56:45, h
On 2020/04/13 16:03:00, hahnjo wrote:
> On 2020/04/13 15:06:16, hanwenn wrote:
> >
https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc
> > File lily/grob-scheme.cc (left):
> >
> >
>
https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc#oldcode326
> > l
https://codereview.appspot.com/555610043/diff/557670044/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):
https://codereview.appspot.com/555610043/diff/557670044/lily/stencil-integral.cc#newcode471
lily/stencil-integral.cc:471:
unrelated change
https://codereview.appspot.com/5556
I gave this patch a try on my system, time lilypond MSDM.ly, 'real'
time, 3 repetitions
baseline (master, 0e7c26d40f):
0m42,533s; 0m42,547s; 0m42,430s
Abstract Grob property storage into Mutable_properties.
(https://codereview.appspot.com/561640043)
0m41,102s; 0m41,036s; 0m40,861s
Rewrite Mutabl
Reviewers: hahnjo,
Message:
On 2020/04/13 16:40:34, hahnjo wrote:
> I gave this patch a try on my system, time lilypond MSDM.ly, 'real'
time, 3
> repetitions
>
> baseline (master, 0e7c26d40f):
> 0m42,533s; 0m42,547s; 0m42,430s
>
> Abstract Grob property storage into Mutable_properties.
> (https:
On 2020/04/13 16:58:41, hanwenn wrote:
> On 2020/04/13 16:40:34, hahnjo wrote:
> > I gave this patch a try on my system, time lilypond MSDM.ly, 'real'
time, 3
> > repetitions
> >
> > baseline (master, 0e7c26d40f):
> > 0m42,533s; 0m42,547s; 0m42,430s
> >
> > Abstract Grob property storage into Mut
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 {
On 2020/04/13 16:58:41, hanwenn wrote:
> On 2020/04/13 16:40:34, hahnjo wrote:
> > I gave this patch a try on my system, time lilypond MSDM.ly, 'real'
time, 3
> > repetitions
> >
> > baseline (master, 0e7c26d40f):
> > 0m42,533s; 0m42,547s; 0m42,430s
> >
> > Abstract Grob property storage into Mut
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 {
"PangoMatrix is a struct, so there is no value in using inheritance" is
a non-sequitur. A struct just is a class with public members.
The motivation for choosing inheritance here was that this allows
passing Transform directly into Pango for operations. This advantage is
not realised in our curr
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
On 2020/04/13 17:28:16, Dan Eble wrote:
> 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
On 2020/04/13 17:17:31, hanwenn wrote:
> On 2020/04/13 16:58:41, hanwenn wrote:
> > On 2020/04/13 16:40:34, hahnjo wrote:
> > > I gave this patch a try on my system, time lilypond MSDM.ly,
'real' time, 3
> > > repetitions
> > >
> > > baseline (master, 0e7c26d40f):
> > > 0m42,533s; 0m42,547s; 0m42,
There is scm_c_memq for avoiding hare&tortoise, but for this use case,
the explicit code seems like the saner option. Even without
hare&tortoise, the list would be consed together each time without
further measures.
https://codereview.appspot.com/579570043/diff/577750045/lily/stencil-integral.cc
https://codereview.appspot.com/57643/diff/577750046/flower/include/offset.hh
File flower/include/offset.hh (right):
https://codereview.appspot.com/57643/diff/577750046/flower/include/offset.hh#newcode123
flower/include/offset.hh:123: Offset normal() const {
It's kind of unusual to make a
On 2020/04/13 17:43:17, hanwenn wrote:
> On 2020/04/13 17:17:31, hanwenn wrote:
> > On 2020/04/13 16:58:41, hanwenn wrote:
> > > On 2020/04/13 16:40:34, hahnjo wrote:
> > > > I gave this patch a try on my system, time lilypond MSDM.ly,
'real' time,
> 3
> > > > repetitions
> > > >
> > > > baseline
This is likely a timing fluke due to thermal throttling too.
Hold off on reviewing.
https://codereview.appspot.com/583750043/
https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly
File input/regression/system-start-brace-style.ly (right):
https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly#newcode4
input/regression/system-st
Reviewers: dak,
https://codereview.appspot.com/57643/diff/577750046/flower/include/offset.hh
File flower/include/offset.hh (right):
https://codereview.appspot.com/57643/diff/577750046/flower/include/offset.hh#newcode123
flower/include/offset.hh:123: Offset normal() const {
On 2020/04/13
https://codereview.appspot.com/579570043/diff/577750045/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):
https://codereview.appspot.com/579570043/diff/577750045/lily/stencil-integral.cc#newcode115
lily/stencil-integral.cc:115: && (SCM_EQ_P (head, ly_symbol2scm
("moveto"))
On 2020/
On 2020/04/13 19:08:31, hanwenn wrote:
>
https://codereview.appspot.com/57643/diff/577750046/flower/include/offset.hh
> File flower/include/offset.hh (right):
>
>
https://codereview.appspot.com/57643/diff/577750046/flower/include/offset.hh#newcode123
> flower/include/offset.hh:123: Offset
On 2020/04/13 19:38:02, dak wrote:
> On 2020/04/13 19:08:31, hanwenn wrote:
> >
>
https://codereview.appspot.com/57643/diff/577750046/flower/include/offset.hh
> > File flower/include/offset.hh (right):
> >
> >
>
https://codereview.appspot.com/57643/diff/577750046/flower/include/offset.hh#n
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 {
https://codereview.appspot.com/575990043/diff/577740043/lily/mutable-properties.cc
File lily/mutable-properties.cc (right):
https://codereview.appspot.com/575990043/diff/577740043/lily/mutable-properties.cc#newcode50
lily/mutable-properties.cc:50: if (SCM_EQ_P (key (i), k))
scm_is_eq
https://co
On 2020/04/13 20:21:29, dak wrote:
>
https://codereview.appspot.com/575990043/diff/577740043/lily/mutable-properties.cc#newcode109
> lily/mutable-properties.cc:109: Mutable_properties::mark () const
> This can segfault in GUILEv2 and/or depending on some other
circumstances since
> the C++ members
On 2020/04/13 20:10:35, dak wrote:
>
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
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#newcode54
lily/include/mutable-properties.hh:54: Iterator iter()
On 2020/04/13 20:27:34, hanwenn wrote:
> On 2020/04/13 20:10:35, dak wrote:
> >
>
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/i
Please give reasons for changes, ie why isn't it necessary to do a deep
copy? Is it currently not failing a test, are the shallow copies
immutable anyway, is there some other guarantee? IMO pushing less
changes with strictly better descriptions will get us further in the
long run.
https://coderevi
On 2020/04/13 20:44:47, hahnjo wrote:
> Please give reasons for changes, ie why isn't it necessary to do a
deep copy? Is
> it currently not failing a test, are the shallow copies immutable
anyway, is
> there some other guarantee?
Done.
https://codereview.appspot.com/561640045/
it's hard to say if this makes a measurable difference:
$ grep -C1 User.time timing-deepcopy.txt
Command being timed: "lilypond -I carver MSDM"
User time (seconds): 56.16
System time (seconds): 0.73
--
Command being timed: "./out/bin/lilypond.baseline -I carver MSD
commit 2d40ef8ced63607cc1a9b2a0bc6a1fd1122af8c4
Author: Han-Wen Nienhuys
Date: Fri Mar 27 19:20:32 2020 +0100
mf: use python scripting for generating Emmentaler fonts
https://codereview.appspot.com/553580043/
On 2020/04/13 20:26:10, dak wrote:
> On 2020/04/13 20:21:29, dak wrote:
>
> >
>
https://codereview.appspot.com/575990043/diff/577740043/lily/mutable-properties.cc#newcode109
> > lily/mutable-properties.cc:109: Mutable_properties::mark () const
> > This can segfault in GUILEv2 and/or depending on s
https://codereview.appspot.com/575990043/diff/577740043/lily/mutable-properties.cc
File lily/mutable-properties.cc (right):
https://codereview.appspot.com/575990043/diff/577740043/lily/mutable-properties.cc#newcode50
lily/mutable-properties.cc:50: if (SCM_EQ_P (key (i), k))
On 2020/04/13 20:21:2
On 2020/04/13 21:13:42, hanwenn wrote:
> >
> > You need to sort the C++ structures potentially accessed by the GC
hook into a
> > Preinit class (grep for Preinit for existing uses) and inherit from
that
> before
> > inheriting from whatever operates the Smob. Then C++ guarantees an
> > initialis
On Apr 13, 2020, at 16:31, hanw...@gmail.com wrote:
>
>> Does const serve a purpose here? The iterator doesn't carry through
> with any
>> kind of enforcement. The same question applies to try_retrieve() and
>> to_alist().
>
> It allows one to iterate over properties in a const method.
>
> Wha
On 4/13/20, 6:17 AM, "lilypond-devel on behalf of David Kastrup"
wrote:
David Kastrup writes:
> Han-Wen Nienhuys writes:
>
>> On Tue, Feb 11, 2020 at 10:17 PM David Kastrup wrote:
>>
>>> > the reason being that it is better if the source code looks like plain
Carl Sorensen writes:
> On 4/13/20, 6:17 AM, "lilypond-devel on behalf of David Kastrup"
> d...@gnu.org> wrote:
>
> It looks to me like we have only two people who have weighed in on the patch:
> the author (David K.) and one reviewer (Han-Wen). Since we have only two
> votes (1 for, 1 agai
On 4/13/20, 4:38 PM, "David Kastrup" wrote:
Carl Sorensen writes:
> On 4/13/20, 6:17 AM, "lilypond-devel on behalf of David Kastrup"
> d...@gnu.org> wrote:
>
> It looks to me like we have only two people who have weighed in on the
patch: the author (David K.) and o
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 23:07:57, Dan Eble wrote:
> 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_mac
I am in favor of this patch because of the following:
1) David K. has a long history of improving things through changes like
this.
2) It does not change the user interface or any files that a user
accesses
3) David has done the work of making all the code changes this syntax
change requires
Carl
On 2020/04/13 18:51:22, hanwenn wrote:
> This is likely a timing fluke due to thermal throttling too.
>
> Hold off on reviewing.
Shall we put the patch on Needs_work then?
https://codereview.appspot.com/583750043/
On 2020/04/12 11:05:14, hanwenn wrote:
> this should probably not be merged, given the inability to demonstrate
speed
> savings. But I'm uploading the latest version to not leave bugs in
the public
> record.
Shall we put the patch on Needs_work to avoid progressing in the usual
process?
https://
63 matches
Mail list logo
