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://
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/
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 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
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 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
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, 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
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 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
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 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
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/
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
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/
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: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
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: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
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
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
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 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
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
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/
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/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
This is likely a timing fluke due to thermal throttling too.
Hold off on reviewing.
https://codereview.appspot.com/583750043/
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
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
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
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,
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: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
"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
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 {
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
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:
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
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
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
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 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
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
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
LGTM
https://codereview.appspot.com/557680043/
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/
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
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
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:
This should very probably not be part of this patch?
https://codereview.a
>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/
If the function is not meant to be implemented / used, it should be
explicitly deleted (C++11).
https://codereview.appspot.com/557680043/
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
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
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
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
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
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
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
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
63 matches
Mail list logo
