Re: Stop smobifying Transform (issue 567580043 by hanw...@gmail.com)

[dak]

On 2020/05/09 16:19:59, hanwenn wrote:
> On Sat, May 9, 2020 at 4:56 PM <mailto:d...@gnu.org> wrote:
> >
> > On 2020/05/09 12:00:01, hanwenn wrote:
> > > On 2020/05/09 11:03:12, dak wrote:
> > > > Rationale?  This negates work done as part of issue 5347 with
the
> > long-term
> > > goal
> > > > of making transforms a better integrated and accessible part of
> > stencils.
> > > This
> > > > will be increasingly important when we move to Cairo since Cairo
> > cannot
> > > > represent rotations by multiples of 90 degrees cleanly other
than by
> > using an
> > > > explicit transform matrix since (in defiance of all other
graphical
> > systems
> > > like
> > > > Postscript, PDF, METAPOST) it refuses to represent angles in any
> > other form
> > > than
> > > > as radians (an actual patch implementing angles as degrees was
> > proferred and
> > > > rejected on what can hardly be called anything but philosophical
> > grounds), and
> > > > PI does not have an exact representation in those.
> > >
> > > Could you sketch how SCM-accessible Transform help with
implementing
> > Cairo
> > > support?
> >
> > I have not said that SCM-accessible transforms help with
implementing
> > Cairo support.  The problem with Cairo is that it cannot properly
> > represent rotations by multiple of 90 degrees (a common use case)
except
> > by using transform matrices, and so the in-stencil (and thus
> > Scheme-accessible) support of transformations, currently mostly
> > constrained to translations and rotations, would have to be moved
over
> > to whole transform matrices.
> >
> > > As you can see from the current state, it is not necessary to
export
> > them to
> > > Scheme, and
> > > I actually think the state from before
> > https://codereview.appspot.com/344970043
> > > is preferable
> > > to SCM encapsulation. With SCM encapsulation, we create GC
overhead,
> > and lose
> > > the type checking that C++ gives us.
> >
> > That statement is not borne out by facts since a Simple_smob does
not
> > incur _any_ GC or Scheme overhead or loses any of the type checking
of
> > C++ unless you actually call smobbed_copy on it.  Overhead occurs
once
> 
> Well, the change I we are discussing, you are actually calling
> smobbed_copy, for example in line 412 here:
>
https://codereview.appspot.com/344970043/diff/60001/lily/stencil-integral.cc

Which actually is a pretty good use of it since passing around an SCM
expression (and the stencil expression being passed in parallel also is
an SCM expression) is cheaper than passing a whole transform matrix by
value as done before.  And there is only rather occasional change needed
(necessitating the creation of a new object) since most stencil
expressions do not change the current transform.

> 
> > > It is extremely hard to  deprecate or change APIs once they gain
use
> > in Scheme,
> > > so everything being equal, the safe option is to not make them
> > accessible IMO.
> >
> > They aren't available from Scheme as of yet.  This is just done in
> > preparation of changes that are sensible to make in future.  And
stencil
> > expressions, albeit expressed in terms of Scheme data structures,
don't
> > really are manipulated to any significant degree from the user.
> >
> > Which is good since it would make a whole lot of sense to implement
them
> > in terms of, say, Cairo data structures in the long run.  And the
> > refactoring of the transform matrix and its use (at the current
point in
> > terms of a Pango transform matrix, but changing that detail would be
> > comparatively trivial even though annoyingly the respective
structures
> > don't have the same internal layout) lends itself to that purpose
very
> > well in that regard.
> >
> > As I stated: performance cannot possibly be an issue before
smobbed_copy
> > () is even called since the memory layout and management and access
is
> > completely unchanged until one does.
> >
> > So undoing work that has been done for the sake of future
extensibility
> > would warrant an actually valid reason.  In the long run, I'd be in
> > preference of moving the SCM reflection of data types now
represented by
> > class derivation from Simple_smob (namely expressions without an SCM
> > identity that sensibly can be eq?-compared and a straightforward
storage
> > layout) to a templated mechanism that allows confining the Scheme
> > reflection to files actually conc

Re: Stop smobifying Transform (issue 567580043 by hanw...@gmail.com)

[dak]

On 2020/05/09 12:00:01, hanwenn wrote:
> On 2020/05/09 11:03:12, dak wrote:
> > Rationale?  This negates work done as part of issue 5347 with the
long-term
> goal
> > of making transforms a better integrated and accessible part of
stencils. 
> This
> > will be increasingly important when we move to Cairo since Cairo
cannot
> > represent rotations by multiples of 90 degrees cleanly other than by
using an
> > explicit transform matrix since (in defiance of all other graphical
systems
> like
> > Postscript, PDF, METAPOST) it refuses to represent angles in any
other form
> than
> > as radians (an actual patch implementing angles as degrees was
proferred and
> > rejected on what can hardly be called anything but philosophical
grounds), and
> > PI does not have an exact representation in those.
> 
> Could you sketch how SCM-accessible Transform help with implementing
Cairo
> support? 

I have not said that SCM-accessible transforms help with implementing
Cairo support.  The problem with Cairo is that it cannot properly
represent rotations by multiple of 90 degrees (a common use case) except
by using transform matrices, and so the in-stencil (and thus
Scheme-accessible) support of transformations, currently mostly
constrained to translations and rotations, would have to be moved over
to whole transform matrices.

> As you can see from the current state, it is not necessary to export
them to
> Scheme, and 
> I actually think the state from before
https://codereview.appspot.com/344970043
> is preferable
> to SCM encapsulation. With SCM encapsulation, we create GC overhead,
and lose
> the type checking that C++ gives us.  

That statement is not borne out by facts since a Simple_smob does not
incur _any_ GC or Scheme overhead or loses any of the type checking of
C++ unless you actually call smobbed_copy on it.  Overhead occurs once
you do significant manipulation with it using Scheme, and when doing so,
there would be a reason for it offsetting the overhead.  The Simple_smob
base class itself contains no virtual functions and no non-static data
members and thus comes without any runtime overhead until you actually
use it from within Scheme.

> It is extremely hard to  deprecate or change APIs once they gain use
in Scheme,
> so everything being equal, the safe option is to not make them
accessible IMO.

They aren't available from Scheme as of yet.  This is just done in
preparation of changes that are sensible to make in future.  And stencil
expressions, albeit expressed in terms of Scheme data structures, don't
really are manipulated to any significant degree from the user.

Which is good since it would make a whole lot of sense to implement them
in terms of, say, Cairo data structures in the long run.  And the
refactoring of the transform matrix and its use (at the current point in
terms of a Pango transform matrix, but changing that detail would be
comparatively trivial even though annoyingly the respective structures
don't have the same internal layout) lends itself to that purpose very
well in that regard.

As I stated: performance cannot possibly be an issue before smobbed_copy
() is even called since the memory layout and management and access is
completely unchanged until one does.

So undoing work that has been done for the sake of future extensibility
would warrant an actually valid reason.  In the long run, I'd be in
preference of moving the SCM reflection of data types now represented by
class derivation from Simple_smob (namely expressions without an SCM
identity that sensibly can be eq?-compared and a straightforward storage
layout) to a templated mechanism that allows confining the Scheme
reflection to files actually concerned with it.

As long as we are not there, however, I don't see the point in making
life harder for those who would want to use transforms from the Scheme
side.  We have enough graphical entities represented by Scheme lists
already, an appallingly expensive storage representation for material of
fixed size.

Having a way to represent transforms with a single SCM cell rather than
6 cons cells and 6 float cells can avert exactly the situation you are
concerned with: too much use of SCM data management for the sake of
getting a job done.

https://codereview.appspot.com/567580043/

Stop smobifying Transform (issue 567580043 by hanw...@gmail.com)

[dak]

Rationale?  This negates work done as part of issue 5347 with the
long-term goal of making transforms a better integrated and accessible
part of stencils.  This will be increasingly important when we move to
Cairo since Cairo cannot represent rotations by multiples of 90 degrees
cleanly other than by using an explicit transform matrix since (in
defiance of all other graphical systems like Postscript, PDF, METAPOST)
it refuses to represent angles in any other form than as radians (an
actual patch implementing angles as degrees was proferred and rejected
on what can hardly be called anything but philosophical grounds), and PI
does not have an exact representation in those.

https://codereview.appspot.com/567580043/

Re: define-markup-command-internal -> module-define-markup-command! (issue 547920045 by d...@gnu.org)

[dak]

Reviewers: hanwenn,

Message:
On 2020/05/08 17:12:41, hanwenn wrote:
> I don't understand how this approach could ever help byte-compiling
the markup
> scheme files. This still uses module-define! , so the guile2
compilation step
> will be oblivious to  markup functions.

It makes it easier to create specific different behavior for
byte-compilation using eval-when.

Description:
define-markup-command-internal -> module-define-markup-command!

Tangible benefits of this approach are not all that clear, and it
doesn't cover define-markup-list-command in the current form either,
so it warrants more work before committing or even deciding to commit.
Particularly so since for convenience it reverts a previous patch
combining the internals of define-markup-command and
define-markup-list-command, in order to illustrate the approach just
on the former.

However, it works and provides a basis for discussion.  The principal
idea is to provide a version of define-markup-command that is not
specific to the current module and not a macro, in a similar vein to
how module-define!  complements define , and use this in the parser
rather than the previous somewhat fuzzier
define-markup-command-internal .

Additional commits:

Add Lily_lexer::current_scope () function


Revert "Express define-markup-list-command-internal using
define-markup-command-internal"

This reverts commit 9f1683921621b612b94080d506ee317b058b29c8.

Please review this at https://codereview.appspot.com/547920045/

Affected files (+40, -22 lines):
  M lily/include/lily-imports.hh
  M lily/include/lily-lexer.hh
  M lily/lily-imports.cc
  M lily/lily-lexer.cc
  M lily/parser.yy
  M scm/markup-macros.scm


Index: lily/include/lily-imports.hh
diff --git a/lily/include/lily-imports.hh b/lily/include/lily-imports.hh
index 
145b5aa763dc544a083bf85d1b3521aed280b988..d6b6e568a2c8e1d30463a84b8138ac9328265f63
 100644
--- a/lily/include/lily-imports.hh
+++ b/lily/include/lily-imports.hh
@@ -67,7 +67,6 @@ extern Variable car_less;
 extern Variable chordmodifiers;
 extern Variable construct_chord_elements;
 extern Variable default_time_signature_settings;
-extern Variable define_markup_command_internal;
 extern Variable drum_pitch_names;
 extern Variable grob_compose_function;
 extern Variable grob_offset_function;
@@ -102,6 +101,7 @@ extern Variable midi_program;
 #if !GUILEV2
 extern Variable module_export_all_x;
 #endif
+extern Variable module_define_markup_command_x;
 extern Variable f_parser;
 extern Variable percussion_p;
 extern Variable pitchnames;
Index: lily/include/lily-lexer.hh
diff --git a/lily/include/lily-lexer.hh b/lily/include/lily-lexer.hh
index 
83fb6f7a2d39dfd28d8ac8f4e7a0b1d38f29ecff..61d9ed6704993490d1a0d1f6a61ecbc786bb3915
 100644
--- a/lily/include/lily-lexer.hh
+++ b/lily/include/lily-lexer.hh
@@ -89,6 +89,7 @@ public:
 
   void add_scope (SCM);
   SCM set_current_scope ();
+  SCM current_scope () const;
   bool has_scope () const;
   SCM remove_scope ();
 
Index: lily/lily-imports.cc
diff --git a/lily/lily-imports.cc b/lily/lily-imports.cc
index 
844a8210ee71da92095c4d3329a39f3503df0f9b..b6a1e28f783ebd9ea9634c02ff7019f12a789798
 100644
--- a/lily/lily-imports.cc
+++ b/lily/lily-imports.cc
@@ -58,7 +58,6 @@ Variable car_less ("car<");
 Variable chordmodifiers ("chordmodifiers");
 Variable construct_chord_elements ("construct-chord-elements");
 Variable default_time_signature_settings ("default-time-signature-settings");
-Variable define_markup_command_internal ("define-markup-command-internal");
 Variable drum_pitch_names ("drumPitchNames");
 Variable grob_compose_function ("grob::compose-function");
 Variable grob_offset_function ("grob::offset-function");
@@ -93,6 +92,7 @@ Variable midi_program ("midi-program");
 #if !GUILEV2
 Variable module_export_all_x ("module-export-all!");
 #endif
+Variable module_define_markup_command_x ("module-define-markup-command!");
 Variable f_parser ("%parser");
 Variable percussion_p ("percussion?");
 Variable pitchnames ("pitchnames");
Index: lily/lily-lexer.cc
diff --git a/lily/lily-lexer.cc b/lily/lily-lexer.cc
index 
4851349cd08f90d04c2f86211bfaa45ea43206f6..6fcec1bca9ab1c17e02eaef79d47a536ae350b89
 100644
--- a/lily/lily-lexer.cc
+++ b/lily/lily-lexer.cc
@@ -152,6 +152,7 @@ Lily_lexer::add_scope (SCM module)
 
   set_current_scope ();
 }
+
 bool
 Lily_lexer::has_scope () const
 {
@@ -180,6 +181,15 @@ Lily_lexer::set_current_scope ()
   return old;
 }
 
+SCM
+Lily_lexer::current_scope () const
+{
+  if (scm_is_pair (scopes_))
+return scm_car (scopes_);
+  error (_ ("no active module in scope"));
+  return SCM_BOOL_F;
+}
+
 int
 Lily_lexer::lookup_keyword (const string &s)
 {
@@ -233,7 +243,7 @@ Lily_lexer::start_main_input ()
 
   new_input (main_input_name_, sources_);
 
-  scm_module_define (scm_car (scopes_),
+  scm_module_define (current_scope (),
  ly_symbol2scm ("input-file-name"),
  ly_string2scm (main_input_name_));
 }
@@ -273,7 +283,7 @@ Lily_lexer::set_id

Re: Split glyph contours in up/down segments for skylines (issue 569700043 by hanw...@gmail.com)

[dak]

On 2020/05/08 14:14:47, hahnjo wrote:

> assert(false) that none of the previous cases was true? That should
detect a
> breaking change in FT_Outline pretty quickly.

else if (condition)
{ ... }
else assert(false);

seems to make the assertion output comparatively useless.  Wouldn't it
be better to do such things as

else {
  assert(condition);
  ...
}

instead?

https://codereview.appspot.com/569700043/

Re: Use GhostScript API instead of forking (issue 548030043 by jonas.hahnf...@gmail.com)

[dak]

On 2020/05/03 10:46:49, hahnjo wrote:
> On 2020/05/03 10:40:25, dak wrote:
> > On 2020/05/03 09:46:56, hahnjo wrote:
> > > rebase + allow -dgs-api=#f to still fork
> > 
> > I decided to look up the terms of the Affero GPL version 3.  The
relevant
> > differences are:
> > [...]
> 
> I think you're not looking at the right direction, ie we're not
integrating GPL
> (LilyPond) into AGPL (Ghostscript), but the other way around. In that
case
> Section 13 of the GPL 3.0 is relevant:
> 
> > Notwithstanding any other provision of this License, you have
permission to
> link or combine any covered work with a work licensed under version 3
of the GNU
> Affero General Public License into a single combined work, and to
convey the
> resulting work. The terms of this License will continue to apply to
the part
> which is the covered work, but the special requirements of the GNU
Affero
> General Public License, section 13, concerning interaction through a
network
> will apply to the combination as such.
> 
> Note there is no mention of modifying the work, AGPL applies "to the
combination
> as such".

Well, if it states "the special requirements of the AGPL apply to the
combination" and the AGPL explicitly says that the combination remains
licensed individually, it definitely sounds to me that the the
requirement to distribute the source of _both_ components triggers when
the AGPL component gets modified.

Artifex states on its web site <https://artifex.com/licensing/agpl/>:

Network Use

> If you modify our software and make the functionality of the software
available to users interacting with it remotely through a computer
network (such as a SaaS or web-based application) you must share your
application source code.

> Any modified versions must also be made available to those interacting
with the software remotely.

To me that sounds like modification of the GPLed part does not count as
modification of the AGPLed part.  If we distribute a combined binary
package, of course we have to state that the package is not, as a whole,
governed by the rules of the GPL since any modification of the AGPLed
part would trigger the requirement to make the combined source available
to network users of the software.

The FSF's GPL FAQ (which of course is not legally binding) states
<https://www.gnu.org/licenses/gpl-faq.html#AGPLv3InteractingRemotely>:
In AGPLv3, what counts as “interacting with [the software] remotely
through a computer network?” (#AGPLv3InteractingRemotely)

If the program is expressly designed to accept user requests and
send responses over a network, then it meets these criteria. Common
examples of programs that would fall into this category include web and
mail servers, interactive web-based applications, and servers for games
that are played online.

If a program is not expressly designed to interact with a user
through a network, but is being run in an environment where it happens
to do so, then it does not fall into this category. For example, an
application is not required to provide source merely because the user is
running it over SSH, or a remote X session.

Again, that sounds to me like the AGPL requirements of using Ghostscript
as an integrated component of web-based PDF generation would not be
different when using Ghostscript unmodified from the command line as
opposed to linked unmodified into a command-line version (modified or
not) of LilyPond.  There would be a conceivable difference if the server
itself were linked with the GS API.

At any rate: my take on this involved evaluating legalese and making a
judgment on it and on the associated risks and possible interpretations.
 But that's not my job, and it shouldn't be ours.

The main thing we can do here is be transparent.  That basically leaves
the responsibility of making decisions with or without contact to
Artifex to the respective users wanting to provide a web service.  And
to be honest: I don't really see that using the GS API is making a
significant difference in that regard.  I think that would have been a
requirement anyway.

https://codereview.appspot.com/548030043/

Re: Use GhostScript API instead of forking (issue 548030043 by jonas.hahnf...@gmail.com)

[dak]

On 2020/05/02 10:22:15, hahnjo wrote:
>
https://codereview.appspot.com/548030043/diff/559960055/lily/general-scheme.cc
> File lily/general-scheme.cc (right):
> 
>
https://codereview.appspot.com/548030043/diff/559960055/lily/general-scheme.cc#newcode778
> lily/general-scheme.cc:778: free (a);
> On 2020/05/02 10:15:40, hanwenn wrote:
> > the code mixes setting up the GS instance (memory management etc)
with
> handling
> > the file. Does it have to be this way? Can we have a 
> > 
> >   class Ghostscript {
> >  process(string file, string device);
> >  close();
> >   };
> > 
> >   Ghostscript *get_gs(vector args);
> > 
> > instead?
> > 
> > I think it should be possible to construct the API such that we
always have
> > ly:gs , and that it falls back to shelling out to GS if the API is
not
> > available.
> 
> No, because there are two types of arguments when using the API: args
and
> device_args where the latter is added to command below. This uses a
different
> syntax and some properties are called differently (HWResolution vs -r
for
> example).

It may well be that calling them "HWResolution" instead of -r would work
fine and -r is just provided as a courtesy abbreviation.  I don't know
whether this holds true, just bringing this up in case it might make for
a different view of the situation.

https://codereview.appspot.com/548030043/

Re: Use GhostScript API instead of forking (issue 548030043 by jonas.hahnf...@gmail.com)

[dak]

On 2020/05/03 09:46:56, hahnjo wrote:
> rebase + allow -dgs-api=#f to still fork

I decided to look up the terms of the Affero GPL version 3.  The
relevant differences are:

> Notwithstanding any other provision of this License, if you modify the
Program, your modified version must prominently offer all users
interacting with it remotely through a computer network (if your version
supports such interaction) an opportunity to receive the Corresponding
Source of your version by providing access to the Corresponding Source
from a network server at no charge, through some standard or customary
means of facilitating copying of software. This Corresponding Source
shall include the Corresponding Source for any work covered by version 3
of the GNU General Public License that is incorporated pursuant to the
following paragraph.

Note that we don't modify Ghostscript (I hope).

> Notwithstanding any other provision of this License, you have
permission to link or combine any covered work with a work licensed
under version 3 of the GNU General Public License into a single combined
work, and to convey the resulting work. The terms of this License will
continue to apply to the part which is the covered work, but the work
with which it is combined will remain governed by version 3 of the GNU
General Public License.

That does not sound like LilyPond would get "infected" by the AGPL.  Not
does it sound like any conditions would trigger unless someone modifies
Ghostscript.  Frankly, this almost sounds like the license was written
explicitly in a manner where it would not get in the hair of GhostScript
API users.

However, what this does mean is that we very much should avoid to be
modifying the Ghostscript sources in any manner, including any patches. 
As long as we don't modify Ghostscript, it sounds like we (and, by the
way, Scorio.com) should be free of other obligations (except of course
the normal GPLv3 terms) even when linking with it.

Of course, we should still prominently mention that this binary contains
AGPL parts.  But I think we could link with the GS API by default and
enable it if that makes sense.

https://codereview.appspot.com/548030043/

Re: Use GhostScript API instead of forking (issue 548030043 by jonas.hahnf...@gmail.com)

[dak]

On 2020/05/02 09:49:44, hahnjo wrote:
> On 2020/05/01 06:28:56, hanwenn wrote:
> > I suggest we make this an option that you have enable explicitly.
> 
> done
> 
> > If it is enabled, we'd have to change the --license output to say
AGPL as
> well.
> 
> I thought about this and decided against it:
> 1. LilyPond stays under GPL, even if the whole may be AGPL.
> 2. I don't see a good option to find out the license of the library we
link to.
> 3. It's not our business of giving legal advice. Instead I added a
warning to
> the configure option "Beware of licensing implications!"

If it is a compile time option that actually links stuff (and the
headers suggest that), we should add something like

This version of LilyPond has been compiled and linked with a version
of Ghostscript licensed under the AGPL.

to the --license text.  The implications are then for the user to find
out.
We should also add this to the license information in the documentation
(namely that --license should give you the information whether there is
the Ghostscript API linked in which case additional terms and conditions
apply).

Yes, this kind of stuff is a pain.  But it's probably less pain in the
end to
be proactive about it.  Our main duty is making sure that users know
what terms
they are dealing with so that the buck stops at the user rather than at
us.

https://codereview.appspot.com/548030043/

Re: Use GhostScript API instead of forking (issue 548030043 by jonas.hahnf...@gmail.com)

[dak]

On 2020/05/01 22:19:38, hanwenn wrote:
> On Fri, May 1, 2020 at 11:58 PM <mailto:v.villen...@gmail.com> wrote:
> >
> > On 2020/05/01 12:12:40, dak wrote:
> > > That being said, the situation regarding Scorio, a proprietary
entity
> > using Free
> > > Software as a component of delivering a web-based service with
> > non-disclosed
> > > components, is _exactly_ the reason Artifex chose the AGPL as a
basis
> > for their
> > > business model selling commercial Ghostscript licenses.
> >
> > And since there is no (as far as I know) contractual, or even simply
> > practical, obligation tying us to Scorio, this is _also_ exactly why
> > LilyPond itself ought to follow the same way IMO.
> >
> > Relicensing LilyPond as AGPL for the future 2.22 branch (or even 3.0
if
> > that, combined with all current optimization work, may be as good a
> > reason as any for a major version bump) should be on a the table,
even
> > independently of the GhostScript situation. (That goes for many
> > free-software programs actually, which date back from before the
SaaS
> > trend and should now consider switching to AGPL; there even were
some
> > talks of merging AGPL into GPLv4 in the future, though the past few
> > months have put many things on hold.)
> 
> I don't want to relicense LilyPond under the AGPL.
> 
> -- 
> Han-Wen Nienhuys - mailto:hanw...@gmail.com -
http://www.xs4all.nl/~hanwen

I am actually with Han-Wen here.  Ultimately, a license should draw the
line you are willing to insist on.  Pursuing GPL breaches is already a
tricky proposition (and essentially only reasonable for significant
copyright holders: the FSF cannot pitch in here since they have no
standing).  Tracking down distributions of LilyPond in a legally
convincing manner is hard enough.  Evidence for running as a service is
just too ephemeral.  I don't want to go there myself.

https://codereview.appspot.com/548030043/

Re: Use GhostScript API instead of forking (issue 548030043 by jonas.hahnf...@gmail.com)

[dak]

On 2020/05/01 11:44:05, hahnjo wrote:
> On 2020/05/01 11:41:13, dak wrote:
> > On 2020/05/01 11:18:11, hahnjo wrote:
> > >
>
https://codereview.appspot.com/548030043/diff/583830043/lily/general-scheme.cc
> > > File lily/general-scheme.cc (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/548030043/diff/583830043/lily/general-scheme.cc#newcode783
> > > lily/general-scheme.cc:783: command += "(" + ly_scm2string (input)
+ ")
> run";
> > > On 2020/05/01 06:42:43, hanwenn wrote:
> > > > This doesn't hook very deeply into GS internals or how we
arrange the
> page.
> > > > Could we get the same speedup by putting the batching into an
encompassing
> > .ps
> > > > file, and calling GS on that file once?
> > > 
> > > I think this would require substantial refactoring of how LilyPond
works. As
> > far
> > > as I understand, currently each input file is handled separately
and
> temporary
> > > files are deleted after processing. To get similar speedup, we
have to reuse
> > the
> > > interpreter for the complete run of LilyPond.
> > > 
> > > > Also: is there a GPL'd version of Ghostscript for which this
works?
> > > 
> > > GPL Ghostscript 9.06 from 2012 is apparently the last GPL version.
You
> > probably
> > > don't want to stick with dependencies of that age ;-)
> > 
> > It would need to be GPLv3+ anyway.  Haven't checked, though.
> > 
> > preview-latex
<https://www.gnu.org/software/auctex/preview-latex.html> works
> by
> > feeding scraps of PostScript control through stdin.  The main thing
is that
> > startup and font loading are not once per graphics but per document.
 I have
> > little enough clue how this would work for us, but obviously any way
of using
> a
> > single Ghostscript session for multiple snippet conversions,
including just a
> > large batch of input files generating lots of small output files
with a
> > predictable naming scheme (this is what LaTeX/Ghostscript under
control of
> > preview.sty can do) will get us significant benefits.  The
Ghostscript API is
> > just one way of organising this.
> 
> Sure, we could also open an "interactive" gs command and keep it
around for the
> process. I didn't consider this different from using the API, but if
the license
> is happy as long as it's a separate process...

I haven't looked too closely at the details of Ghostscript but the
cursory license information from Artifex I have seen was not
specifically talking about the API.  I have not looked closer though. 
This is more of a question of "what are we prepared to get sued over". 
The legal situation with APIs and/or linking is at best murky.  Many
proprietary offerings work around that by casting themselves in contract
form, basically stating "you may not even use the software unless you
agree to those terms", and those define what is considered a
combined/derived work in the sense of the contract rather than the sense
of the law.  For us the line likely is not as much the law but when
Artifex is likely to spring in action.

That being said, the situation regarding Scorio, a proprietary entity
using Free Software as a component of delivering a web-based service
with non-disclosed components, is _exactly_ the reason Artifex chose the
AGPL as a basis for their business model selling commercial Ghostscript
licenses.

So they quite likely are prepared to threaten legal action over it.  We
just have to make sure that they aren't going to target us, and
preferably that it's actually easy to get a commercially licensed
Ghostscript version working in that context so that parties like
Scorio.com have the option to do so and know the conditions where it
makes sense for them to take out that option.

I don't see the API as the trigger point judging from the Ghostscript
licensing page, but I may well be overlooking something here.



https://codereview.appspot.com/548030043/

Re: Use GhostScript API instead of forking (issue 548030043 by jonas.hahnf...@gmail.com)

[dak]

On 2020/05/01 11:18:11, hahnjo wrote:
>
https://codereview.appspot.com/548030043/diff/583830043/lily/general-scheme.cc
> File lily/general-scheme.cc (right):
> 
>
https://codereview.appspot.com/548030043/diff/583830043/lily/general-scheme.cc#newcode783
> lily/general-scheme.cc:783: command += "(" + ly_scm2string (input) +
") run";
> On 2020/05/01 06:42:43, hanwenn wrote:
> > This doesn't hook very deeply into GS internals or how we arrange
the page.
> > Could we get the same speedup by putting the batching into an
encompassing .ps
> > file, and calling GS on that file once?
> 
> I think this would require substantial refactoring of how LilyPond
works. As far
> as I understand, currently each input file is handled separately and
temporary
> files are deleted after processing. To get similar speedup, we have to
reuse the
> interpreter for the complete run of LilyPond.
> 
> > Also: is there a GPL'd version of Ghostscript for which this works?
> 
> GPL Ghostscript 9.06 from 2012 is apparently the last GPL version. You
probably
> don't want to stick with dependencies of that age ;-)

It would need to be GPLv3+ anyway.  Haven't checked, though.

preview-latex 
works by feeding scraps of PostScript control through stdin.  The main
thing is that startup and font loading are not once per graphics but per
document.  I have little enough clue how this would work for us, but
obviously any way of using a single Ghostscript session for multiple
snippet conversions, including just a large batch of input files
generating lots of small output files with a predictable naming scheme
(this is what LaTeX/Ghostscript under control of preview.sty can do)
will get us significant benefits.  The Ghostscript API is just one way
of organising this.

https://codereview.appspot.com/548030043/

Re: Use a hash table for the lexer keywords (issue 549920043 by hanw...@gmail.com)

[dak]

On 2020/05/01 11:15:29, hanwenn wrote:
> On 2020/04/30 07:58:09, dak wrote:
> > On 2020/04/30 07:42:16, hahnjo wrote:
> > > On 2020/04/27 11:58:11, dak wrote:
> > > > Tracker issue: 5946
> (https://sourceforge.net/p/testlilyissues/issues/5946/)
> > > > Rietveld issue: 577840053
(https://codereview.appspot.com/577840053)
> > > > Issue description:
> > > >   Use Scheme_hash_table for keyword handling
> > > 
> > > We should probably decide between these two approaches, we likely
can't do
> > both.
> > > I see the advantage of using SCM values for everything, but I'm
not familiar
> > > with the code.
> > 
> > I think it makes more sense here not to introduce new data types for
a job
> that
> > is inherent to Scheme's operation.  Having both patches on countdown
at the
> same
> > time obviously does not make sense: if discussion is required
(Han-Wen?), it
> > would make sense to stop both until this is resolved.
> > 
> > An independent component is the removal of ly:lexer-keywords . 
There is no
> > indication that it ever has been used; it is cheap to provide with
my version.
> 
> > However, revisiting its code I also see that it takes a lexer as an
argument. 
> > My patch, like Han-Wen's, stops lexers from having their individual
keytable
> (an
> > implementation detail that was never used for any purpose).  So even
if the
> > function were retained, letting it take an argument, while making
for
> backwards
> > compatibility, does not appear to make sense.  This could be
addressed in a
> > separate patch/issue.  Or it could be removed in a separate
patch/issue.
> > 
> > One possible use for it would be using LilyPond itself for
generating syntax
> > highlighting for editors.  The current solutions rather extract
stuff from the
> > source I seem to remember.
> 
> 1) we can take dak's patch if you prefer. They're roughly equivalent
anyway.
> 
> 2) I suppose ly:lexer-keywords was for editor modes, but it hasn't
been used for
> that.
> Updating the scripts to generate the modes is extra work, so I vote
for removing
> it. If 
> nobody bothered in the last 13 years, it can't be that important.

I'll make the removal a separate issue (and obviously commit) once the
"base patch" is in.  That makes it possible to find, reference, and
possibly revert the removal in case that is ever wanted.

https://codereview.appspot.com/549920043/

Re: Use GhostScript API instead of forking (issue 548030043 by jonas.hahnf...@gmail.com)

[dak]

On 2020/05/01 07:58:32, hahnjo wrote:
> disclaimer: I'm not a lawyer, this is just my understanding of the
licenses.
> 
> On 2020/05/01 06:28:56, hanwenn wrote:
> > Technologically speaking, this is a brilliant idea, and I am all in
favor it.
> > 
> > However, I think we can't enable this by default.
> > 
> > Ghostscript is licensed under AGPL, and linking it in makes LilyPond
a derived
> > work, putting it under AGPL as well. That would be effectively a
license
> change
> > of LilyPond, which would need consent of the current authors, and I
think the
> > Scorio folks would not be happy with.
> 
> To put this on a solid basis, here's a quote from
> https://www.gnu.org/licenses/gpl-3.0.html
> 
> 13. Use with the GNU Affero General Public License.
> 
> Notwithstanding any other provision of this License, you have
permission to link
> or combine any covered work with a work licensed under version 3 of
the GNU
> Affero General Public License into a single combined work, and to
convey the
> resulting work. The terms of this License will continue to apply to
the part
> which is the covered work, but the special requirements of the GNU
Affero
> General Public License, section 13, concerning interaction through a
network
> will apply to the combination as such.
> 
> So I disagree that this is a license change as the "terms of this
License will
> continue to apply to the part which is the covered work". LilyPond
stays
> licensed under GPLv3. But yes "the special requirements of the GNU
Affero
> General Public License, section 13, concerning interaction through a
network
> will apply to the combination as such".
> 
> And here's the point that many have long argued about and continue to
disagree:
> What is "combination"? Linking to a library clearly is (and it's even
spelled
> out explicitly), but what about other "control flows" like calling
GhostScript.
> Here's another quote:
> 
> The “Corresponding Source” for a work in object code form means all
the source
> code needed to generate, install, and (for an executable work) run the
object
> code and to modify the work, including scripts to control those
activities.
> However, it does not include the work's System Libraries, or
general-purpose
> tools or generally available free programs which are used unmodified
in
> performing those activities but which are not part of the work. For
example,
> Corresponding Source includes interface definition files associated
with source
> files for the work, and the source code for shared libraries and
dynamically
> linked subprograms that the work is specifically designed to require,
such as by
> intimate data communication or control flow between those subprograms
and other
> parts of the work.
> 
> So the first sentence and the example seem to include runtime
dependencies
> ("subprograms"), but others (like System Libraries) are excluded. I
don't know
> whether this applies to GhostScript, but I'd argue that it's not a
"major
> essential component" of the OS. So in my opinion, LilyPond + GS
already is AGPL
> if you're using a recent version of the software.
> 
> 
> > I suggest we make this an option that you have enable explicitly. If
it is
> > enabled, we'd have to change the --license output to say AGPL as
well.
> 
> I'm open to making this change if people prefer. The major benefit
will be for
> documentation builds anyway, so the "normal" usage of LilyPond is fine
with
> forking.

>From Artifex' web site regaring licensing::

If you meet certain criteria, then we will not consider your
distribution of your application in an executable form to be in
violation of AGPL, even though you ship an executable product that
includes your application and Ghostscript. Here are the criteria:

It is conspicuous and clear to the end user that he/she is getting
access to two separate pieces of software (i.e., AGPL Ghostscript in
addition to the application using this product).
The end user has the ability to opt out of installing the AGPL
version of our products during the install process.
Each AGPL module is separable and replaceable within the build.
The available source code for the AGPL modules must be for the build
that corresponds with your binaries.

Any other redistribution is not licensed under AGPL. If you cannot meet
the requirements of AGPL, please fill out the form below to inquire
about a commercial license.

It is pretty likely that if software like Scorio.com wanted to avail
themselves of the advantages of the Ghostscript API performance, they
would need to adhere to the AGPL terms or take out a commercial license
of Ghostscript.  We definitely should not be distributing LilyPond in a
form where it would only work using the GhostScript API, but with a
separate option that is off by default, I don't see much of a problem.

https://codereview.appspot.com/548030043/

Re: Use GhostScript API instead of forking (issue 548030043 by jonas.hahnf...@gmail.com)

[dak]

On 2020/04/30 19:15:23, hahnjo wrote:
> This probably goes without saying, but touching such core
functionality should
> receive very rigid testing. I hope I got most things covered, but
especially
> manual feedback would be very much appreciated!

Sounds pretty good.  A great long-term perspective would be to use Cairo
for everything (or almost everything).  Last time I looked, the PDF
document model was insufficient for getting all LilyPond PDF features
supported, but that may have changed.

LilyPond's internal graphical data structures are certainly worth
replacing with something more compact.  If stencils were just Cairo
data...

https://codereview.appspot.com/548030043/

Re: Use a hash table for the lexer keywords (issue 549920043 by hanw...@gmail.com)

[dak]

On 2020/04/30 07:42:16, hahnjo wrote:
> On 2020/04/27 11:58:11, dak wrote:
> > Tracker issue: 5946
(https://sourceforge.net/p/testlilyissues/issues/5946/)
> > Rietveld issue: 577840053 (https://codereview.appspot.com/577840053)
> > Issue description:
> >   Use Scheme_hash_table for keyword handling
> 
> We should probably decide between these two approaches, we likely
can't do both.
> I see the advantage of using SCM values for everything, but I'm not
familiar
> with the code.

I think it makes more sense here not to introduce new data types for a
job that is inherent to Scheme's operation.  Having both patches on
countdown at the same time obviously does not make sense: if discussion
is required (Han-Wen?), it would make sense to stop both until this is
resolved.

An independent component is the removal of ly:lexer-keywords .  There is
no indication that it ever has been used; it is cheap to provide with my
version.  However, revisiting its code I also see that it takes a lexer
as an argument.  My patch, like Han-Wen's, stops lexers from having
their individual keytable (an implementation detail that was never used
for any purpose).  So even if the function were retained, letting it
take an argument, while making for backwards compatibility, does not
appear to make sense.  This could be addressed in a separate
patch/issue.  Or it could be removed in a separate patch/issue.

One possible use for it would be using LilyPond itself for generating
syntax highlighting for editors.  The current solutions rather extract
stuff from the source I seem to remember.

https://codereview.appspot.com/549920043/

Re: Use a hash table for the lexer keywords (issue 549920043 by hanw...@gmail.com)

[dak]

Regarding the "Get rid of unused function ly:lexer-keywords": that one
was added with
commit 1ae6f5710e714f13da1bfbfbd840835316618d1f
Author: Han-Wen Nienhuys 
Date:   Thu Dec 14 14:10:56 2006 +0100

add ly:lexer-keywords to make keyword extraction easier and exact.

but there appears to be no commit that ever used it.  The sentiment
expressed in the commit message appears reasonable, and with the
alternative proposal in issue 5946, the implementation comes basically
free.  Do you remember what you were planning to do with it at the time
you committed it?

https://codereview.appspot.com/549920043/

Issue 5934: remove Repeated_music::folded_music_length (issue 561670043 by nine.fierce.ball...@gmail.com)

[dak]

Interesting.  In 2.18 it appears to be used as length-callback in
TremoloRepeatedMusic.  Was that wrong or did something change in
unfolded_music_length ?

https://codereview.appspot.com/561670043/

Re: Use Scheme_hash_table for keyword handling (issue 577840053 by d...@gnu.org)

[dak]

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.
On 2020/04/27 23:24:19, Dan Eble wrote:
> Is this what you've just done, or is there still more?

Will remove.  Not uploading separate patch just for that, though.

https://codereview.appspot.com/577840053/

Re: Use Scheme_hash_table for keyword handling (issue 577840053 by d...@gnu.org)

[dak]

On 2020/04/27 23:07:17, dak wrote:
> On 2020/04/27 22:57:06, dak wrote:
> > Move keytable init out of constructor.  Frankly, I don't consider
this an
> > improvement.
> 
> As anecdotal data, it took me about as long to get this working as the
whole
> other patch, and I had it segfault about half a dozen times and had
another
> half-dozen compiler errors while getting stuff right.
> 
> And doc_hash_table, all_ifaces and option_hash are initialised in the
same
> manner as what I did in the previous patch version.  And it's not like
lexer
> startup time is really going to factor in here a lot...  The previous
code
> generated an individual key table for every single lexer (except for
cloned
> lexers).
> 
> So without further feedback, I strongly lean towards pushing the
previous
> version.

Though I'd have no problems pulling out the initialisation into a
_private_ static member function (can be called from the constructor
obviously) if people consider it desirable to factor out that part of
the initialisation.  Does not need to access keytable_ but can just
return the finished table.

https://codereview.appspot.com/577840053/

Re: Use Scheme_hash_table for keyword handling (issue 577840053 by d...@gnu.org)

[dak]

On 2020/04/27 22:57:06, dak wrote:
> Move keytable init out of constructor.  Frankly, I don't consider this
an
> improvement.

As anecdotal data, it took me about as long to get this working as the
whole other patch, and I had it segfault about half a dozen times and
had another half-dozen compiler errors while getting stuff right.

And doc_hash_table, all_ifaces and option_hash are initialised in the
same manner as what I did in the previous patch version.  And it's not
like lexer startup time is really going to factor in here a lot...  The
previous code generated an individual key table for every single lexer
(except for cloned lexers).

So without further feedback, I strongly lean towards pushing the
previous version.

https://codereview.appspot.com/577840053/

Re: Use Scheme_hash_table for keyword handling (issue 577840053 by d...@gnu.org)

[dak]

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_;
On 2020/04/27 21:22:59, hanwenn wrote:
> could move out of Lily_lexer.

It's only used in there and changing it elsewhere would be really bad,
so why?

https://codereview.appspot.com/577840053/diff/561760044/lily/lily-lexer.cc#newcode102
lily/lily-lexer.cc:102: if (!keytable_.is_bound ())
On 2020/04/27 21:22:59, hanwenn wrote:
> move this to a global init function? There is no reason to this in the
> constructor.

It has to be done when the Scheme system is up and before the first call
of a lexer.  I'll take a look at moving it outside; skeptical it will be
a win in readability/trustworthiness.

In order to keep access to keytable_ private, the initialization will
need to get done by a static member function.

https://codereview.appspot.com/577840053/

Re: Thread skyline construction through stencil interpretation (issue 581960043 by hanw...@gmail.com)

[dak]

That's the kind of stuff that warrants compiling a few scores with
-ddebug-skylines and looking rather closely for kinks.


https://codereview.appspot.com/581960043/diff/561710044/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

https://codereview.appspot.com/581960043/diff/561710044/lily/stencil-integral.cc#newcode245
lily/stencil-integral.cc:245: for (vsize i = 0; i < 8; i += 2)
On 2020/04/27 19:56:59, hahnjo wrote:
> hm, can we avoid hardcoding the size twice - or even get rid of it
completely? I
> think you can write
> Offset points[] = { ... };
> and
> i < sizeof(points) / sizeof(points[0])

Offset points[2][] = { ... };
for (auto &i : points)
  skyline->add_segment (transform, i[0], i[1]);

May want to be auto i, I am not yet all too acquainted with C++11
hotness.

https://codereview.appspot.com/581960043/diff/561710044/lily/stencil-integral.cc#newcode253
lily/stencil-integral.cc:253: vector points;
Seems unused.

https://codereview.appspot.com/581960043/

Re: Use a hash table for the lexer keywords (issue 549920043 by hanw...@gmail.com)

[dak]

On 2020/04/27 09:11:16, dak wrote:
> Han-Wen Nienhuys <mailto:hanw...@gmail.com> writes:
> 
> > On Mon, Apr 27, 2020 at 10:59 AM <mailto:d...@gnu.org> wrote:
> >> When the parser sees some \blabla it will generally first have to
check
> >> for a keyword and (when it has no match) afterwards for a variable
with
> >> that name, and a lot of those are actually music functions these
days
> >> that used to be keyword.
> >>
> >> Wouldn't it make sense to just convert \xxx into a symbol early on
in
> >> the lexer?  That is essentially a hash code, and we have lookups
for
> >> those.  It seems wasteful to use two completely different ways of
> >> hashing a string in succession when we can just turn this into a
symbol
> >> early on and work with that instead.
> >
> > If you can make Bison work off SCM symbol values, be my guest.
> 
> Yo may have missed the memo, but everything passed into and out of the
> parser and Lexer these days is SCM.  And for looking up a token ID
from
> an SCM symbol, Guile has hashtables, and we also habe Scm_hashtable.
> 
> -- 
> David Kastrup

Tracker issue: 5946
(https://sourceforge.net/p/testlilyissues/issues/5946/)
Rietveld issue: 577840053 (https://codereview.appspot.com/577840053)
Issue description:
  Use Scheme_hash_table for keyword handling


https://codereview.appspot.com/549920043/

Re: Use a hash table for the lexer keywords (issue 549920043 by hanw...@gmail.com)

[dak]

Without having looked all that much at the code:

When the parser sees some \blabla it will generally first have to check
for a keyword and (when it has no match) afterwards for a variable with
that name, and a lot of those are actually music functions these days
that used to be keyword.

Wouldn't it make sense to just convert \xxx into a symbol early on in
the lexer?  That is essentially a hash code, and we have lookups for
those.  It seems wasteful to use two completely different ways of
hashing a string in succession when we can just turn this into a symbol
early on and work with that instead.

https://codereview.appspot.com/549920043/

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[dak]

On 2020/04/24 13:15:10, dak wrote:
> mailto:hanw...@gmail.com writes:
> 
> > Here is how I have experienced this discussion:
> >
> > DAK: this is micro-optimization that causes memory fragmentation.
> >
> > HW: No, it's not; here is a benchmark that shows decreased memory
use as
> > well.
> >
> > DAK: We could use even different data structures in the future.
> >
> > HW: This seems very philosophical to me.
> 
> It is interesting that you call it "philosophical" to want to change
> data structures while getting a patch of yours reviewed that wants to
> change data structures.  In particular after I indicated that I even
> have code.
> 
> Why is it "philosophical" when someone other than you does it?
> 
> > Maybe I misunderstood, because the hardcoding of a specific STL
> > container type predates this patch.
> 
> Changing the used container type would be a reasonable point of time
for
> changing the hardcoding, particularly since it facilitates
comparisons.
> Also C++11 makes this considerably less awkward than previously, and
> code written with lists in mind tends to require less adaptation to
use
> via vectors than the other way round, anyway.

At any rate, this is not "philosophical" really.  The kind of action
done here is also provided by C++' container adaptor std::queue which
can be based off std::list as well as std::deque where the latter is a
compromise in storage use and locality between vectors and lists and
would well worth be checking out as well.

Hard-coding to one particular container type makes this a lot more
expensive and does not allow revisiting the decision for later versions
of compiler/library or actually entirely different compilers.

https://codereview.appspot.com/583750043/

Re: Add a script for running timing benchmarks (issue 545950043 by hanw...@gmail.com)

[dak]

On 2020/04/25 17:07:17, hahnjo wrote:
> I strongly object to adding more random scripts to the source tree.
There are
> already far too many unmaintained in scripts/auxiliar/ with no
documentation at
> all.

How about approaching this in a different manner then?  Adding
instructions to the CG about how to benchmark LilyPond's behavior in a
sensible manner?  And if the instructions end up bothersome to follow,
back them up with scripts doing the bulk of the work?

While I agree that adding more "use-me-if-you-manage-to-find-me"
material is not overly helpful, the basic idea for providing tools for a
common task is certainly not wrong.  And there are contributors that are
more comfortable on starting their work with tasks where the main
channel of feedback is at first provided by computers, meaning that they
don't get the feeling they are taxing anybody's patience with getting
feedback on their first steps.

https://codereview.appspot.com/545950043/

Re: Fix warnings related to po (issue 551830043 by jonas.hahnf...@gmail.com)

[dak]

On 2020/04/25 08:36:02, hahnjo wrote:
> On 2020/04/24 20:14:36, hahnjo wrote:
> > On 2020/04/24 18:25:33, lemzwerg wrote:
> > > LGTM, thanks a lot!
> > > 
> > >
https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile
> > > File po/GNUmakefile (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile#newcode14
> > > po/GNUmakefile:14: sed-makestuff = -e
's/[a-zA-Z_/]*make\[[0-9]*\].*//'
> > > shouldn't this be rather '[0-9]\+'?
> > > 
> > >
> >
>
https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile#newcode15
> > > po/GNUmakefile:15: sed-edstuff = -e 's/[ \.,adic0-9]*//' -e
's/---//' | sort
> > -u
> > > I think that within '[...]' you don't have to escape '.'.
> > > 
> > > And again I would use '\+' instead of '*'.
> > 
> > To be honest, I have no clue what all of this is doing: I copied it
from
> > stepmake/stepmake/podir-targets.make.
> 
> After looking through the code once more, this sed magic is only used
for
> po-changes. This spams the output when running po-update which is
pointless IMO
> because the one updating lilypond.pot is not going to update all
languages at
> the same time. Thus, I'm dropping it in the updated patch set.

At any rate, I don't think the backslash is intended to escape the
period rather than just be literally enclosed in the character list. 
Some people tend to do this in the form of the redundant \\ in order to
not confuse human readers and almost but not quite syntax-aware editors,
but with regard to the actual interpretation, it is not doing anything. 
Not even escaping the backslash itself: something like [A-\\-~] is still
two ranges rather than a single range followed by two isolated
characters, the interpretation that would result from "proper" backslash
escaping.

https://codereview.appspot.com/551830043/

Re: Minor cleanups in stencil-integral.cc (issue 579630043 by hanw...@gmail.com)

[dak]

On 2020/04/25 07:29:06, hanwenn wrote:
> On 2020/04/24 22:04:52, dak wrote:
> > On 2020/04/24 21:33:12, Carl wrote:
> > > On 2020/04/24 21:19:57, dak wrote:
> > > > On 2020/04/24 21:18:12, dak wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc
> > > > > File lily/stencil-integral.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc#newcode465
> > > > > lily/stencil-integral.cc:465: // more convoluted, but it's
fairly hot
> > path.
> > > > > Sorry for not being clear: the question was not why this
change was
> > > effective
> > > > in
> > > > > saving time, but why it was valid.  When thickness is zero,
you only
> > update
> > > > the
> > > > > upper skyline.  Why would the lower skyline no longer need
updating?
> > > > 
> > > > Well, other way round, but apart from that the question stands.
> > > 
> > > When thickness is zero, the upper and lower curves are the same. 
Either one
> > > completes the skyline.
> > 
> > Of course they are the same.  That is not the question.  The
question is why I
> > am considering the identical curve for the minimum skyline while I
don't let
> it
> > participate in the maximum skyline any more.
> > 
> > If everything is of thickness null, the maximum skyline will be
non-existent
> > while the minimum skyline is there.  While the skylines should be
identical
> > rather than only one of them being there.
> 
> the Direction d is up/down, but note that it calculates d *
> normal(curve-direction), so you get to two curves that correspond to
the outer
> and inner curve of a stroked bezier.
> 
> The curve has no particular orientation relative to the axes, so
-assuming the
> previous code was correct- it can't be that the UP points are for the
UP
> skyline.

Right.  At the end of the function, both curves are added into boxes
without orientation.  Which begs the question why they are stored into a
points array in the first place.  Adding into successive boxes requires
just remembering the respective last point pair.

Also some conditions read "th > 0" while others read "th == 0" and they
depend on one another for their behavior.  There is nothing that
precludes negative values in this code, so it seems like all tests
should be consistent (not quite sure what the implications of negative
th should be: one could argue forcing to zero as well as just taking the
absolute value).

> 
> You can see this happening here near the bottom of the function: the
points are
> line segments, and then each line segment is interpreted as a box. The
boxes are
> then added to the curve.
> 
> I think this all can be more succinctly expressed by just calculating
the curve
> once and fattening the resulting box by .5 thickness on all sides, but
all the
> same, it would be better to get rid of the boxes entirely, and work
from curves
> directly to skylines. 
> 
> I have plans to do this, but they require overhaul of calling
conventions in the
> stencil -> {box,building} -> skyline pipeline.



https://codereview.appspot.com/579630043/

Re: Rewrite Skyline code (issue 547980044 by hanw...@gmail.com)

[dak]

On 2020/04/19 22:35:39, dak wrote:
> On 2020/04/19 21:43:26, hanwenn wrote:
> > (this still needs some work, but the speedup might be worth an early
look)
> 
> * Buildings store Y coordinate of the left edge, rather than the
>   intercept at x==0.0.  This avoid excessive rounding problems when X
>   is large, and lets us compute building.height_at(x[LEFT]) cheaply.
> 
> Since intercept computation order depends on the execution of the
merge
> algorithm and we use floating point arithmetic rather than fixed
point, this
> would make results a lot less reproducible.
> 
> I understand the sentiment and thought about it as well, but it would
only work
> if the _original_ coordinates were retained even after intersection
and
> additional intersections still worked based on the original
coordinates, making
> the merge order (mostly) irrelevant with regard to error propagation.

Actually, one way to keep this pretty order independent would be just
not to intersect until the final pass and use the preceding passes just
for throwing out those line segments that are completely shadowed by
others.  This should not keep all that much more data but would have to
deal with a larger current working set where max/min relations are
checked.  I have no clear view on the expected performance impact.

https://codereview.appspot.com/547980044/

Re: Minor cleanups in stencil-integral.cc (issue 579630043 by hanw...@gmail.com)

[dak]

On 2020/04/24 21:33:12, Carl wrote:
> On 2020/04/24 21:19:57, dak wrote:
> > On 2020/04/24 21:18:12, dak wrote:
> > >
> >
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc
> > > File lily/stencil-integral.cc (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc#newcode465
> > > lily/stencil-integral.cc:465: // more convoluted, but it's fairly
hot path.
> > > Sorry for not being clear: the question was not why this change
was
> effective
> > in
> > > saving time, but why it was valid.  When thickness is zero, you
only update
> > the
> > > upper skyline.  Why would the lower skyline no longer need
updating?
> > 
> > Well, other way round, but apart from that the question stands.
> 
> When thickness is zero, the upper and lower curves are the same. 
Either one
> completes the skyline.

Of course they are the same.  That is not the question.  The question is
why I am considering the identical curve for the minimum skyline while I
don't let it participate in the maximum skyline any more.

If everything is of thickness null, the maximum skyline will be
non-existent while the minimum skyline is there.  While the skylines
should be identical rather than only one of them being there.

https://codereview.appspot.com/579630043/

Re: Minor cleanups in stencil-integral.cc (issue 579630043 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc#newcode465
lily/stencil-integral.cc:465: // more convoluted, but it's fairly hot
path.
Sorry for not being clear: the question was not why this change was
effective in saving time, but why it was valid.  When thickness is zero,
you only update the upper skyline.  Why would the lower skyline no
longer need updating?

https://codereview.appspot.com/579630043/

Re: Minor cleanups in stencil-integral.cc (issue 579630043 by hanw...@gmail.com)

[dak]

On 2020/04/24 21:18:12, dak wrote:
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc
> File lily/stencil-integral.cc (right):
> 
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc#newcode465
> lily/stencil-integral.cc:465: // more convoluted, but it's fairly hot
path.
> Sorry for not being clear: the question was not why this change was
effective in
> saving time, but why it was valid.  When thickness is zero, you only
update the
> upper skyline.  Why would the lower skyline no longer need updating?

Well, other way round, but apart from that the question stands.

https://codereview.appspot.com/579630043/

Re: make_draw_bezier_boxes: save work if thickness == 0.0 (issue 551730043 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc#newcode496
lily/stencil-integral.cc:496: break;
I think that is less readable than desirable.  Instead how about putting

if (th == 0.0)
   break;

behind the push?  That makes very obvious that only one point is being
pushed without further cleverness.

Either way you'll be checking thickness twice if its non-zero.  The
trivial way to avoid that would be to simply unfold the loop.  Like

points[DOWN].push_back (scm_transform (trans, curve.control_[0] + DOWN *
normal);
if (th != 0.0)
  points[UP].push_back (scm_transform (trans, curve.control_[0] + UP *
normal;

Written like that, it actually becomes far from trivial to see why this
optimisation would be valid, so maybe add a comment explaining it for
the sake of human readers?

https://codereview.appspot.com/551730043/

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[dak]

On 2020/04/23 16:01:20, dak wrote:
> On 2020/04/22 07:43:17, hanwenn wrote:

> > * The linked list has an overhead of 2 pointers, on a 4x Real
> > datastructure, ie. 50% overhead.

By the way: C++11 has Forward_list which would be another plugin
container type with less overhead.

I actually have some sort routine for that lying around here that I
offered several times on the libc++ list without pickup.  Someone
benchmarked it to be about 30% faster than what's in there, on average
(and, as its a merge sort, with a worst case hardly worse) but I gave up
trying to forcefeed it to them after a few tries.  Did not take any
extra memory apart from an O(lg n) stack maintained manually rather than
recursively.

Now what would probably work reasonably well is to just collect
buildings without bothering to look at them, clean up via a priority
queue when one has gathered enough material and merge ultimately. 
Priority queues can be well done with vectors.

All of those more order-ignorant methods will want arithmetic that is
largely order-independent.

https://codereview.appspot.com/583750043/

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[dak]

On 2020/04/22 07:43:17, hanwenn wrote:
> current master:
> 
> 4f906b95aea99f2a47d5ba037f7421e5bb933c42 (origin/master) Issue 5908:
> get_path_list: use loop instead of tail recursion
> 
> Command being timed: "out/bin/lilypond.4f906b95ae -I carver MSDM"
> User time (seconds): 34.30
> Maximum resident set size (kbytes): 1139208
> 
> this patch:
> 
> 889131d584f77f44acc8d801ce254d7d2ba85aa4 (HEAD, vector-skyline) Use
> vectors rather than lists for skylines.
> Command being timed: "out/bin/lilypond.889131d584 -I carver MSDM"
> User time (seconds): 31.66
> Maximum resident set size (kbytes): 1054432
> 
> 
> I disagree David's assessment on several theoretical grounds too:
> 
> * "less maintainable manner with hand-written optimisations". I don't
> see these in this patch. This (together with the Tie_performer) is the
> only place in LilyPond that uses lists. We could get rid of std::list
> on maintenance grounds alone.

This patch may not be the best illustration of the problem, but it does
leave something to be desired as well.  When the flow and functionality
functionality of the skyline code here does not depend on whether one
uses vectors or lists, the actual exchange should be of one typedef. 
Then one is free to change the implementation at a whim and when other
conditions may change, like changes in the memory support.  C++ is a
painful language for one reason: not sacrificing functionality while
still being able to separate data types and algorithms in a modular
manner.  We pay the price for using C++ so we should also reap the
rewards in usability.  STL provides a unified interface to containers
for a reason.

> 
> * The linked list has an overhead of 2 pointers, on a 4x Real
> datastructure, ie. 50% overhead. The vector has (assuming a 2x growth
> strategy) 100% overhead. There is a little more overhead, but we could
> tune that by adjustin the vector growth strategy. With a linked list,
> there is no choice in space/time tradeoff.

When I read "adjustin the vector growth strategy", that again sounds
like microoptimisation by replacing STL algorithms with something
homespun.  That just makes no sense since it ultimately will not buy us
more than about 30% of performance while locking us into a code base
that can neither be easily maintained nor brought up to speed in case
STL improves or we want to plug in, say, a Boost library.  If we want to
close LilyPond to further development, squeezing the last 30% of
performance out in return for lots worse in maintainability.

> * The linked list approach is much worse for fragmentation. It has to
> allocate a ton on 48-byte objects, some of which have long lifetime.
> This will create a highly fragmented pool of 48-byte objects. We don't
> have use for so many of these objects elsewhere. By contrast, the
> vector approach will create a distribution of differently sized
> objects, which can be recycled into other vectors.

Vectors are usually grown in fixed growth factors and the elements of
the vectors here are not something with a straightforward size such SCM.
 So we have similar problems with vectors.

At any rate, if the code were written agnostic with regard to the
actually used container, there would be no need to burn a final decision
into code and one could revisit at some future time.  Or write
yet-another-container that does a better job at merging structures with
not automatically balanced subdivisions.

I actually do have some half-finished code for that sitting somewhere
that postpones merges of significantly different sized subskylines.  One
of the problem areas was, unhard to guess, ensuring results that would
not (or not significantly) depend on merge order for numeric reasons
because that makes things awfully irreproducible.

https://codereview.appspot.com/583750043/

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[dak]

I am rather skeptical about the usefulness of such microoptimizations
without 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 filled and processed, so it would
rather seem like the true savings are not to be found in doing the same
kind of work in a slightly faster but less maintainable manner with
hand-written optimisations, but rather in figuring out why too much work
is being done in the first place.

The more one replaces standard tools and operations, the harder it
becomes to figure out what kind of stuff actually goes wrong and fix it,
or change the strategies and algorithms wholesale.

https://codereview.appspot.com/583750043/

Re: Obviate method_finder methods (issue 551780043 by d...@gnu.org)

[dak]

On 2020/04/21 00:43:02, dak wrote:
> On 2020/04/20 23:26:07, Dan Eble wrote:
> > 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.
> 
> Not without a lot of effort since MFP1_WRAP and MFP2_WRAP rely on
"trampoline"
> in the current lexical scope doing something sensible.  They are
unclean macros.
>  So one would have to overhaul the whole trampoline concept to collect
all
> trampolines in one scope (either :: or Callback?::) and scatter
specialisations
> over the files that want them.  A lot of work for what does not really
look like
> a whole lot of gain.

Also if you were thinking of turning MFP0_WRAP into a _function_ taking
a pointer to member function argument, that will not work since the
whole trampoline creation concept relies on the particular pointer to
member being a template argument and thus fixed.  There is a separate
trampoline generated for each member function.

https://codereview.appspot.com/551780043/

Re: Obviate method_finder methods (issue 551780043 by d...@gnu.org)

[dak]

On 2020/04/20 23:26:07, Dan Eble wrote:
> 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.

Not without a lot of effort since MFP1_WRAP and MFP2_WRAP rely on
"trampoline" in the current lexical scope doing something sensible. 
They are unclean macros.  So one would have to overhaul the whole
trampoline concept to collect all trampolines in one scope (either :: or
Callback?::) and scatter specialisations over the files that want them. 
A lot of work for what does not really look like a whole lot of gain.

https://codereview.appspot.com/551780043/

Issue #1204: fix font-name-add-files regtest (issue 573730044 by v.villen...@gmail.com)

[dak]

https://codereview.appspot.com/573730044/diff/583810043/input/regression/font-name-add-files.ly
File input/regression/font-name-add-files.ly (right):

https://codereview.appspot.com/573730044/diff/583810043/input/regression/font-name-add-files.ly#newcode25
input/regression/font-name-add-files.ly:25: tmpdir = #(let ((env-tmpdir
(getenv "TMPDIR")))
tmpdir = #(or (getenv "TMPDIR") "/tmp")

https://codereview.appspot.com/573730044/diff/583810043/input/regression/font-name-add-files.ly#newcode28
input/regression/font-name-add-files.ly:28: dummyname = #(port-filename
(mkstemp! (string-append tmpdir "/" "dummyfont-XX")))
This fixes the problem that mkstemp! needs a writable string since
string-append returns a newly allocated string.  Whether it's by design
or not the computer won't care, so that should fix the potential
Guile-2.x problem.

https://codereview.appspot.com/573730044/diff/583810043/input/regression/font-name-add-files.ly#newcode252
input/regression/font-name-add-files.ly:252: (if (or (equal? "." f)
No point in a functional language to write as if expressions and
procedures were different things.  Just write

(or (equal? "." f) (equal? ".." f)
(delete-file (string-append ...)))

https://codereview.appspot.com/573730044/

Re: Rewrite Skyline code (issue 547980044 by hanw...@gmail.com)

[dak]

On 2020/04/19 21:43:26, hanwenn wrote:
> (this still needs some work, but the speedup might be worth an early
look)

* Buildings store Y coordinate of the left edge, rather than the
  intercept at x==0.0.  This avoid excessive rounding problems when X
  is large, and lets us compute building.height_at(x[LEFT]) cheaply.

Since intercept computation order depends on the execution of the merge
algorithm and we use floating point arithmetic rather than fixed point,
this would make results a lot less reproducible.

I understand the sentiment and thought about it as well, but it would
only work if the _original_ coordinates were retained even after
intersection and additional intersections still worked based on the
original coordinates, making the merge order (mostly) irrelevant with
regard to error propagation.

https://codereview.appspot.com/547980044/

Re: Obviate method_finder methods (issue 551780043 by d...@gnu.org)

[dak]

On 2020/04/19 11:03:35, hahnjo wrote:
> On 2020/04/19 10:55:52, dak wrote:
> I have the vague
> > impression that the ability to do template specialisation on
typedefs would
> also
> > be able to solve part of the job here in a more elegant manner but
right now
> my
> > brain objects to more C++ style.
> 
> Yes, I had the same feeling. However I think that the current solution
works
> because the template arguments are deduced from the function argument.
I don't
> currently see a way to make this work with template <> using type =,
but that
> would certainly be nice (and remove the need of using decltype and
> remove_pointer).

The principal problem is that the trampoline instantiation requires the
member function pointer as a non-type template parameter, and the type
of the non-type template parameter is not known in general.  So this
becomes a hen-and-egg problem.  Template specialisation could possibly
weasel around it but I am not sure about it.  It certainly is suspicious
that the otherwise rather comprehensive  relying on such
techniques has nothing in its toolbox for that task.

https://codereview.appspot.com/551780043/

Re: Obviate method_finder methods (issue 551780043 by d...@gnu.org)

[dak]

On 2020/04/19 09:55:03, hahnjo wrote:
> On 2020/04/19 09:39:00, dak wrote:
> >
>
https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh
> > File lily/include/callback.hh (right):
> > 
> >
>
https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh#newcode166
> > lily/include/callback.hh:166: typedef typename
> remove_pointer > (static_cast (nullptr)))>::type type;
> > On 2020/04/19 09:22:44, hahnjo wrote:
> > > I'd consider 'using type = ' instead which is more C++ style
> > 
> > I think you overestimate my C++ fu.  Can you spell this out?
> 
> using type = typename remove_pointer
> (nullptr)))>::type;
> to declare the type alias:
https://en.cppreference.com/w/cpp/language/type_alias

I see.  Basically squeezing in another piece of syntax where it has no
sensible place in order to have a variant of typedef that also works for
template specialisation of types.  Or in other words, more C++ style.  I
have the vague impression that the ability to do template specialisation
on typedefs would also be able to solve part of the job here in a more
elegant manner but right now my brain objects to more C++ style.  So
I'll confine myself to the change you suggest for now.  Will take
probably half the day to get to the next patch as I am also doing some
other reorganisation.

Thanks!

https://codereview.appspot.com/551780043/

Re: Obviate method_finder methods (issue 551780043 by d...@gnu.org)

[dak]

https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh
File lily/include/callback.hh (right):

https://codereview.appspot.com/551780043/diff/573720045/lily/include/callback.hh#newcode166
lily/include/callback.hh:166: typedef typename
remove_pointer (nullptr)))>::type
type;
On 2020/04/19 09:22:44, hahnjo wrote:
> I'd consider 'using type = ' instead which is more C++ style

I think you overestimate my C++ fu.  Can you spell this out?

https://codereview.appspot.com/551780043/

Re: Obviate method_finder methods (issue 551780043 by d...@gnu.org)

[dak]

Reviewers: lemzwerg,

Message:
On 2020/04/19 05:53:34, lemzwerg wrote:
> Nice!  I don't really understand the C++ wizardry, but having to type
less
> macros in the normal code is certainly beneficial.

The wizardry is comparatively confined and mainly consists of a more
polished version of the mfp_baseclass thing that I needed to address
Dan's complaints about issue 5916 (which will need to get rebased on
this more elaborate version).  The advantage, of course, is that the
whole problem underlying issue 5600 that kept Clang from working (and
likely would have stopped GCC at some point of time) before Jonas found
a solution: gone.

The fundamental problem is that something like
&Global_context::create_context_from_event (a member function pointer to
an inherited function) is not of type void (Global_context::*)(SCM) but
of type void (Context::*)(SCM), and in the context of template argument
matching, you need to match the type exactly.

The basic C++ wizardry used here would already have worked in C++98 but
would have required a number of more specialised versions of
mfp_baseclass (for lack of template parameter packs).  I just failed to
come up with it before Dan asked for wart removal in issue 5916.

The disadvantage, of course is "I don't really understand the C++
wizardry".  C++11 has appropriated Perl's capacity of reading like line
noise, but in contrast to Perl, you never really get to the stage where
this line noise stops causing headaches.

At least this one saves a lot of other headaches.  And mfp_baseclass
does a well-defined job in a manner and with an interface typical to
C++11.  And it's few lines of headache in a single place rather than
something spread over all of the source code.

Description:
Obviate method_finder methods

The inheritance of various Translator::method_finder functions was a
comparatively fragile mess.  A helper class mfp_baseclass is able to
figure out the proper baseclass for a given member function pointer.
That allows removing the method_finder functions from the various
Translator-derived classes.

Please review this at https://codereview.appspot.com/551780043/

Affected files (+52, -61 lines):
  M lily/auto-beam-engraver.cc
  M lily/beam-engraver.cc
  M lily/context.cc
  M lily/include/callback.hh
  M lily/include/coherent-ligature-engraver.hh
  M lily/include/engraver.hh
  M lily/include/gregorian-ligature-engraver.hh
  M lily/include/ligature-engraver.hh
  M lily/include/translator.hh
  M lily/include/translator.icc
  M lily/kievan-ligature-engraver.cc
  M lily/mensural-ligature-engraver.cc
  M lily/phrasing-slur-engraver.cc
  M lily/repeat-tie-engraver.cc
  M lily/score-engraver.cc
  M lily/score-performer.cc
  M lily/translator-group.cc
  M lily/vaticana-ligature-engraver.cc

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by d...@gnu.org)

[dak]

On 2020/04/18 14:43:24, Dan Eble wrote:
> 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 moved on.

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.

https://codereview.appspot.com/549890043/

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by d...@gnu.org)

[dak]

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.cc:49: event_source ()->add_listener (GET_LISTENER
(static_cast (this), create_context_from_event),
On 2020/04/18 13:54:30, Dan Eble wrote:
> This is a wart.

Yes and no.  The real wart is that
&Global_context::create_context_from_event is of type Context::* and not
default-convertible to Global_context::* .  That is a property of C++
that has triggered a number of painful consequences elsewhere in our
code base.

>  Without the cast, my version of g++ has this to say:
> 
> ... error: no matching function for call to
>
'Callback_wrapper::make_smob::type,
> &Context::create_context_from_event> > ...

Exactly.

> Notice that it correctly selects the method
Context::create_context_from_event. 
> It should be possible to extract the proper class type in a second
step from
> that instead of directly from the provided pointer.

I tried doing so.  It's complicated, and it only changes a single use
case in all of the code base.  Looked like more of a wart than this is.

> For that, I think you might need to write a template function that
returns the
> class type given the member function pointer as a function parameter;
I don't
> remember if the C++11 standard library has this, and I don't see it as
I skim
> through cppreference.

Probably a few hours of work, for a single use case.  Admittedly going
to that work might also provide a lead towards addressing the other
instances where this C++ problem has given us something to chew on.  I
think that one thing that was impacted was the standoff with Clang
compatibility that Jonas finally addressed by hard-coding a number of
exceptions, in a similar vein.

> I'm sure you won't like complicating your macro,

At the current point of time, there is a single use case.

> but as it stands, someone who
> writes code that is missing a cast has some reading to do, whereas if
you
> complicate the macro to handle this case, they won't have to read it.

I think that if we design a cure for the ::* problem, focusing it on
this single occurence seems like overkill.

I agree that reading it creates itchy fingers.

https://codereview.appspot.com/549890043/diff/573710043/lily/include/listener.hh
File lily/include/listener.hh (right):

https://codereview.appspot.com/549890043/diff/573710043/lily/include/listener.hh#newcode143
lily/include/listener.hh:143: // This is somewhat more official, but way
overkill
On 2020/04/18 13:54:30, Dan Eble wrote:
> Overkill in what respect?  I would use standard features where they
are
> appropriate rather than reinventing them.  Especially in tricky
situations like
> this, using something with a fixed meaning is beneficial.

Overkill in loading a large header file of which only very little
functionality is required.  Now that listener.hh has been removed from a
number of other source files, the impact is more limited and the case
for not using type_traits is weaker.

One thing I have not mentioned speaking for the use of type_traits is
that the line

   (&decltype (dummy_deref (ptr))::proc)> > (),\

would not get accepted as

   &decltype (dummy_deref (ptr))::proc> > (), \

by g++9 (parse error I think) suggesting that this ad-hoc implementation
is sailing uncomfortably close to triggering compiler bugs.  While this
may also be the case with the official header file, it's quite more
probable that problems would be detected and fixed without our
intervention.

At any rate, you are pretty good at retracing the steps and
considerations I resolved on my own.

Ok, I'll let this be done by type_traits alone.  Personally, I consider
it an outrage that decltype (*(ptr)) apparently cannot be made to work
on its own here.  GNU's typeof probably could but it's not part of
--std=c++11

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.

https://codereview.appspot.com/549890043/

Re: get_path_list: use loop instead of tail recursion (issue 579570046 by d...@gnu.org)

[dak]

Reviewers: hanwenn,

Message:
On 2020/04/17 09:12:18, hanwenn wrote:
> LGTM
> 
>
https://codereview.appspot.com/579570046/diff/579580044/lily/stencil-integral.cc
> File lily/stencil-integral.cc (right):
> 
>
https://codereview.appspot.com/579570046/diff/579580044/lily/stencil-integral.cc#newcode111
> lily/stencil-integral.cc:111: for (;scm_is_pair (l); l = scm_cdr (l))

Reformatted.  Not uploading a separate review for this one, though.

Description:
get_path_list: use loop instead of tail recursion

Please review this at https://codereview.appspot.com/579570046/

Affected files (+3, -4 lines):
  M lily/stencil-integral.cc


Index: lily/stencil-integral.cc
diff --git a/lily/stencil-integral.cc b/lily/stencil-integral.cc
index 
6f4dfbae3fee4e764a9a308f290d009e2bb5fae9..449b1ff4a86792350ec21269e4160a43064029f0
 100644
--- a/lily/stencil-integral.cc
+++ b/lily/stencil-integral.cc
@@ -108,7 +108,7 @@ get_number_list (SCM l)
 SCM
 get_path_list (SCM l)
 {
-  if (scm_is_pair (l))
+  for (;scm_is_pair (l); l = scm_cdr (l))
 {
   if (scm_is_true (scm_memv (scm_car (l),
  scm_list_n (ly_symbol2scm ("moveto"),
@@ -121,9 +121,8 @@ get_path_list (SCM l)
  SCM_UNDEFINED
 return l;
   SCM res = get_path_list (scm_car (l));
-  if (scm_is_false (res))
-return get_path_list (scm_cdr (l));
-  return res;
+  if (scm_is_true (res))
+return res;
 }
   return SCM_BOOL_F;
 }

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by d...@gnu.org)

[dak]

On 2020/04/18 10:30:27, hanwenn wrote:
>
https://codereview.appspot.com/549890043/diff/579610043/lily/include/listener.hh
> File lily/include/listener.hh (left):
> 
>
https://codereview.appspot.com/549890043/diff/579610043/lily/include/listener.hh#oldcode140
> lily/include/listener.hh:140: #define GET_LISTENER(cl, proc)
get_listener
> (Callback_wrapper::make_smob > ())
> IMO, the problem is that the macro scopes 'proc' to come from 'cl',
which is
> magical. Couldn't we change the calling convention to be 
> 
>   ctx->GET_LISTENER(&SomeContext::func)

Nope.  For one thing, you should be happy Smob_core no longer needs to
provide a member function just for the sake of Listener, making Listener
less generic and introducing a compile-time dependency.  For another, it
requires the call site to spell out the type of the expression ctx.  And
the third is that the implementation still needs to derive SomeContext
from &SomeContext::func in order to meet the requirements of the
Callback_wrapper cell created.

> if we call this from within a SomeContext method itself, it could be 
> 
>GET_LISTENER(&func)

No, it couldn't.  C++ member function pointer syntax cannot be done in
that manner.
 
> If so, would we need a macro at all?

The main purpose of the macro is not providing a wrapper for the
Listener creator, but rather for creating the template arguments for the
Callback_wrapper magic.  That one's the hardcore bit.  The Listener type
just curries two SCM values.

https://codereview.appspot.com/549890043/

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by d...@gnu.org)

[dak]

On 2020/04/18 10:32:50, lemzwerg wrote:
> > I agree with the sentiment, but I fail to come up
> > with a naming choice that does not make the cure
> > worse than the problem.
> 
> I would simply use GET_LISTENERX (or GET_LISTENER1) for the longer
call.

That's being obtuse.  Requires learning two things instead of one.

https://codereview.appspot.com/549890043/

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by d...@gnu.org)

[dak]

On 2020/04/18 10:26:01, dak wrote:
> On 2020/04/18 05:40:53, lemzwerg wrote:
> 
> > What about using two macros instead of one?  The first would take a
context as a
> > first argument (as it does now), the second one uses 'this' in the
macro body,
> > omitting that as a macro argument.  The former seems to be a rarer
case than the
> > latter.
> 
> Well, the problem is that having GET_LISTENER (... and GET_LISTENER
(this, ...
> does not really take significantly more space than GET_LISTENER (...
and
> GET_SELF_LISTENER (... while being less clear about what is happening.
> 
> Similarly for, say, GET_OUTSIDE_LISTENER vs GET_LISTENER (if you try
to give the
> explicit listener the longer name).
> 
> I agree with the sentiment, but I fail to come up with a naming choice
that does
> not make the cure worse than the problem.

By the way, the last code iteration has removed specific listener
support from the Smob data type.  That means that no part of the
implementation actually is inherently tied to Listener-specific member
functions any more.  With regard to your suggestion, that is probably
not much more than a philosophical consideration.

However, I find the idea of a macro that silently analyses "this" in
order to implicitly create a type in case GET_LISTENER is called from a
member function actually worse than having it given explicitly.

It's like writing something like

  decltype(this) p;  // No guarantee this works

instead of

  Grob *p;

in a member function of Grob.

https://codereview.appspot.com/549890043/

Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by d...@gnu.org)

[dak]

On 2020/04/18 05:40:53, lemzwerg wrote:
> LGTM - but I don't know the internals well enough to really decide
that.

It's really not as much about the internals rather than about how to
express their use.  GET_LISTENER provides an SCM-callable memoized
function port into a C++ member function.  The last iteration of this
code in issue 4357 had to add the type argument to the GET_LISTENER
macro and I was comparatively unhappy about the resulting syntax.  The
change was akin to

-  add_listener (GET_LISTENER (new_context->unset_property_from_event),
+  add_listener (new_context->GET_LISTENER (Context,
unset_property_from_event),

As you can see, I had to split the previous single argument into two and
separately specify the type of the first.  The items that this technique
needs to process are
Context, &Context::unset_property_from_event for creating an
SCM-accessible memoized trampoline function and new_context for
generating an SCM listener port to a specific Smob structure.
> 
> What about using two macros instead of one?  The first would take a
context as a
> first argument (as it does now), the second one uses 'this' in the
macro body,
> omitting that as a macro argument.  The former seems to be a rarer
case than the
> latter.

Well, the problem is that having GET_LISTENER (... and GET_LISTENER
(this, ... does not really take significantly more space than
GET_LISTENER (... and GET_SELF_LISTENER (... while being less clear
about what is happening.

Similarly for, say, GET_OUTSIDE_LISTENER vs GET_LISTENER (if you try to
give the explicit listener the longer name).

I agree with the sentiment, but I fail to come up with a naming choice
that does not make the cure worse than the problem.

https://codereview.appspot.com/549890043/

Re: Use Interval for representing skyline X coordinates (issue 571980043 by hanw...@gmail.com)

[dak]

Purpose is not clear since the code does not appear to use anything
other than the explicit endpoints.

Also, intervals are indexed by LEFT/RIGHT rather than START/STOP. 
Values may be the same, but the former is a whole lot more mnemonic.

https://codereview.appspot.com/571980043/

Re: Rewrite Mutable_properties based on vector (issue 575990043 by hanw...@gmail.com)

[dak]

On 2020/04/13 17:43:17, hanwenn wrote:

> 
> Actually, it looks like I have been measuring noise. Thermal
throttling? 
> 
> It looks like I can get reproducible timings if I downclock the CPU
from 2.6 to
> 2.0Ghz

Put the following line in the config
/etc/modprobe.d/acpi.conf:options thinkpad-acpi fan_control=1

Then you'll be able to do

echo level 7|sudo tee /proc/acpi/ibm/fan

[CPU intensive stuff]

echo level auto|sudo tee /proc/acpi/ibm/fan

There is also level disengaged , but it's a lot more noise (and fan
wear) for rather little gain.  The fans for T61 tend to die a lot more
than those for T400, but removing and reinstalling a fan only from a fan
assembly (also containing heat sink and heat pipe) is more involved than
just exchanging the fan assembly.

https://codereview.appspot.com/575990043/

Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)

[dak]

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_macro_actually(...) and couldn't find the
method in
> klass.hh.  I'm glad future contributors will not have to repeat that
experience.

It's embarrassing to admit, but while I did voice something akin to
making macros imitate member function call syntax, I actually forgot
that I had exactly the same kind of somewhat time-consuming double take
when trying to track down that admittedly clever code.

https://codereview.appspot.com/573670043/

Re: Rewrite Mutable_properties based on vector (issue 575990043 by hanw...@gmail.com)

[dak]

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
> > initialisation order consistent with GC hook operation.
> 
> I read the comment about this, but this only applies to derived
classes, right?
> This is set in the Grob class, which is the base class, so the vector
is init'd
> before smobify_self is called.

think-think-think-think...  I think you are correct.

https://codereview.appspot.com/575990043/

Re: Abstract Grob property storage into Mutable_properties. (issue 561640043 by hanw...@gmail.com)

[dak]

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/include/mutable-properties.hh#newcode31
> > lily/include/mutable-properties.hh:31: class Iterator {
> > On 2020/04/13 17:22:52, hanwenn wrote:
> > > On 2020/04/13 17:15:19, Dan Eble wrote:
> > > > This is useful, but instead of choosing your own method names,
why not
> give
> > it
> > > > an interface consistent with standard iterators (where the
semantics
> match)?
> > > > 
> > > > https://en.cppreference.com/w/cpp/named_req/InputIterator
> > > > 
> > > > I'd leave out the key() and value() methods and let it dealing
with just a
> > > > generic list of SCM values.
> > > > 
> > > > If you implemented enough of the iterator interface, and a class
to wrap
> the
> > > raw
> > > > SCM list into an object addressable with begin() and end(), I
think you
> > would
> > > be
> > > > able to write range-for statements something like this:
> > > > 
> > > > for (SCM assoc : ly_list(my_alist))
> > > >   {
> > > > SCM key = scm_car(assoc);
> > > > SCM val = scm_cdr(assoc);
> > > > ...;
> > > >   }
> > > > 
> > > > 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.
> > 
> > I don't see that there is a plan to get away from the value being an
SCM cell,
> > just the key (at least explicitly).  Am I overlooking something?
> 
> The value itself is not necessarily a cell. You can have #t as a value
(I am
> sure you know that.)

Sure.  Sorry, I assumed you used "cell" colloquially for "SCM" since it
wasn't clear to me that you were talking about the association rather
than their components.

> Making the Key/value combination a cell forces one to store them as
such, or to
> construct a cell on the fly.

Yes, of course.  I thought about key or value being SCM, not about their
association being accessible as a pair or other form of cell.

> There is an experiment where I store keys as uint16.

Sure.

https://codereview.appspot.com/561640043/

Re: Rewrite Mutable_properties based on vector (issue 575990043 by hanw...@gmail.com)

[dak]

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 are only getting initialized after the GC hook is
already
> operative.
> 
> 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
> initialisation order consistent with GC hook operation.

Actually, a less involved implementation where the value cell is SCM is
to just use a Scheme vector.  That also significantly speeds up garbage
collection relying on mark hooks because then only a single scm_gc_mark
call is made and Guile does the rest internally.

https://codereview.appspot.com/575990043/

Re: Rewrite Mutable_properties based on vector (issue 575990043 by hanw...@gmail.com)

[dak]

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://codereview.appspot.com/575990043/diff/577740043/lily/mutable-properties.cc#newcode71
lily/mutable-properties.cc:71: if (SCM_EQ_P (key (i), k))
scm_is_eq

https://codereview.appspot.com/575990043/diff/577740043/lily/mutable-properties.cc#newcode97
lily/mutable-properties.cc:97: if (SCM_EQ_P (key (i), k))
scm_is_eq

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 are only getting initialized after
the GC hook is already operative.

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 initialisation order consistent with GC hook operation.

https://codereview.appspot.com/575990043/

Re: Abstract Grob property storage into Mutable_properties. (issue 561640043 by hanw...@gmail.com)

[dak]

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 17:22:52, hanwenn wrote:
> On 2020/04/13 17:15:19, Dan Eble wrote:
> > This is useful, but instead of choosing your own method names, why
not give it
> > an interface consistent with standard iterators (where the semantics
match)?
> > 
> > https://en.cppreference.com/w/cpp/named_req/InputIterator
> > 
> > I'd leave out the key() and value() methods and let it dealing with
just a
> > generic list of SCM values.
> > 
> > If you implemented enough of the iterator interface, and a class to
wrap the
> raw
> > SCM list into an object addressable with begin() and end(), I think
you would
> be
> > able to write range-for statements something like this:
> > 
> > for (SCM assoc : ly_list(my_alist))
> >   {
> > SCM key = scm_car(assoc);
> > SCM val = scm_cdr(assoc);
> > ...;
> >   }
> > 
> > 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.

I don't see that there is a plan to get away from the value being an SCM
cell, just the key (at least explicitly).  Am I overlooking something?

https://codereview.appspot.com/561640043/

Re: Move get_normal to Offset::normal (issue 576000043 by hanw...@gmail.com)

[dak]

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 normal() const {
> On 2020/04/13 18:24:47, dak wrote:
> > It's kind of unusual to make a constructing function a (non-static)
member
> > function unless this is necessitated for operator semantics, because
different
> > conversion rules apply and there is some expectation that member
functions
> > delivering the class type may change stuff in-place (the const is
not seen at
> > call sites).
> > 
> > Any reason to depart from conventions here?
> 
> It's consistent with offset::direction, offset::swapped, but I
followed your
> suggestion.

You are right regarding direction and swapped, so your proposed change
was not creating precedent and with the reason of maintaining
convention/consistency untenable, I think I prefer your originally
proposed syntax.  Sorry for the noise.

Any input from our C++ gurus?

https://codereview.appspot.com/57643/

Move get_normal to Offset::normal (issue 576000043 by hanw...@gmail.com)

[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 {
It's kind of unusual to make a constructing function a (non-static)
member function unless this is necessitated for operator semantics,
because different conversion rules apply and there is some expectation
that member functions delivering the class type may change stuff
in-place (the const is not seen at call sites).

Any reason to depart from conventions here?

https://codereview.appspot.com/57643/

Optimize get_path_list. (issue 579570043 by hanw...@gmail.com)

[dak]

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
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"))
SCM_EQ_P is an old deprecated macro.  Use scm_is_eq instead.  Same
performance.

https://codereview.appspot.com/579570043/diff/577750045/lily/stencil-integral.cc#newcode127
lily/stencil-integral.cc:127: return get_path_list (scm_cdr (l));
Not caused by this patch, but this does look like something nicer
addressed by a loop rather than tail recursion with regard to C++
idioms.

https://codereview.appspot.com/579570043/

Re: Use composition for embedding PangoMatrix in Transform (issue 555610043 by hanw...@gmail.com)

[dak]

"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 current code base, and doing so does not appear like
the best long-term plan since a good long-term plan would likely rather
involve using the transform matrices of Cairo since it would make a lot
more sense to route the bulk of graphics operations through Cairo than
maintaining our own limited and maintenance-intensive set of backends
and/or relying on Ghostscript as comparatively computationally expensive
intermediary.

So while I don't see any objective of value achieved here (the objective
of not using multiple inheritance went out the window long ago since it
is needed, for example, for determining initialisation order for
GC-marked objects), I don't see anything adversely impacted, either.

https://codereview.appspot.com/555610043/

Re: Define Scheme markups using define-public (issue 577720043 by hanw...@gmail.com)

[dak]

On 2020/04/12 10:59:21, hanwenn wrote:
> On 2020/03/30 12:20:17, dak wrote:
> > > > Ugly and a maintenance burden since the code is used twice. 
Anything
> > > > wrong with my proposal?
> > > 
> > > I didn't understand your proposal.
> > > 
> > > > It does not have duplicate code, makes
> > > > define-markup-command compilable (while requiring its toplevel
use) and
> > > > provides a way of doing the same consistently for
module-specific rather
> > > > than toplevel use.
> > > >
> > > > It sacrifices, like your proposal, non-toplevel-performance for
the sake
> > > > of compilability in Guilev2.  It's just that what the parser
then uses
> > > > is in a form that could also be used in a reasonably natural
manner from
> > > > Scheme.
> > > >
> > > > Should I write up a patch doing that?
> > > 
> > > Yes please.
> > > 
> > 
> > Working on it.  
> 
> Any update?

Sorry, got lost among other stuff.  I have something working in a branch
which I'll rebase and upload presently.  It illustrates the point I was
trying to make though I'll have to admit that this looked more
worthwhile "on paper" so far: I am still not all too clear about how
this would help with the byte compilation situation even though it
cleans up the current situation.  Also, making the call of
(current-module) explicit may help in finding a macro invocation that
delays the actual call until it can actually work.

Also it is more of a sketch, and since the sketch was about
define-markup-command, it starts out reverting your union of
define-markup-command and define-markup-command-list.  Since there is
not much of a point in having different implementations for either, this
is of course something that ultimately needs addressing.

So in short, it's not ready for submission in the current form, but it
certainly spells out what I tried expressing in the above comment.  Even
though I am still fuzzy about whether it will be more helpful in finding
a solution for the byte compilation.

https://codereview.appspot.com/577720043/

Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)

[dak]

On 2020/04/12 10:35:24, hanwenn wrote:
> On Sat, Apr 11, 2020 at 3:55 PM David Kastrup 
wrote:
> > >> You haven't understood the scheme.  A grob only contains the
properties
> > >> seminal to its type.  That's why there is a double indirection. 
A full
> > >> array of 416 entries is needed per grob _type_, not per grob.
> > >
> > > I still don't see why this requires an overall overhaul of the
> > > internal calling convention. You can have a global registry of
symbol
> > > => uint16 (numbering all the symbols),
> >
> > It would be pointless to have a single registry for music
properties,
> > grob properties, stream event properties, and context properties,
thus
> > significantly blowing up the second-level indexing tables and making
for
> > a large number of cache misses.
> 
> there are 400 grob properties, and about 800 properties in total
> (including music and context). 400 requires 9 bits, 800 requires 10
> bits. It doesn't appreciably change the problem size.

Populating arrays densely or sparsely makes a large difference in how
well they fit caches.  However, this is completely irrelevant since this
patch does not, I repeat _not_ enforce any particular implementation. 
All that it does is provide the macros with more complete information
and thus enables implementations that are not crippled down by having
macros pretend to be able to deal with overload resolution.

That you don't feel that you are able to make use of that change does
not mean that you are the only developer who should be allowed to work
on such problems and consequently should be able to block off any
attempt of restructuring the source code in a manner that allows other
developers to more easily experiment with other approaches without
having to constantly resolve rebase conflicts because a design decision
unsuitable for refactoring is kept in place for no good reason.

You'll find that we have had several restructurings of macro call
locations in the past years in order to facilitate refactorings, some
with more, some with less success and permanent impact.  In general, the
developers proposing them had reasonable expectations for the directions
those changes would open for them, and reasonable consideration for the
impact on others.  Please keep in mind that LilyPond, as it stands now,
is a colloborative project.  Accommodating the work of others to
proceed, given reasonable goals and impact, is therefore a necessity.  A
stance that amounts to "I cannot make that work, so we shouldn't move in
a direction where you could try" is not really useful for a
colloborative effort of developers with equal standing.

I had several other proposals of syntax that would provide required
information to the macros in case, but this is the least intrusive one
that retains some flavor of overloading.

https://codereview.appspot.com/573670043/

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[dak]

On 2020/04/11 17:44:11, hanwenn wrote:
> On 2020/04/11 17:19:19, dak wrote:
> > > I thought of removing the other function too, and I agree in
principle, but
> it
> > > can only break more user files. As long as we're not considering
> reorganizing
> > > the immutable lists, I think it can stay.
> > 
> > Note that there is no necessity of returning a bonafide part of the
grob
> > structure: one can construct a list of properties on the fly. 
Particularly
> > because this is used read-only and only occasionally.
> 
> I know. But if we make a copy, that will be expensive too.

It's either imprudent user code or code that actually potentially uses
all the properties.  Either way I'd not be worried about the cost.  That
we don't want to use it in the internals for regular use then is
obvious.

https://codereview.appspot.com/549840044/

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[dak]

On 2020/04/11 16:38:28, hanwenn wrote:
> On 2020/04/11 16:02:31, dak wrote:
> > The description says "[ly:grob-properties] will not return the
properties that
> > were \overridden."
> > 
> > Aren't you confusing this with ly:grob-basic-properties ?  I think
> > ly:grob-properties will actually _only_ return the properties that
were
> > overriden.
> 
> \override pushes data onto the immutable list (or at least, it used to
be like
> that).

In effect you are right (and I misremembered), but override pushes onto
context-maintained lists which are ultimately used as template when
make_grob gets called and _then_ the overriden properties end up on the
immutable list.

> I thought of removing the other function too, and I agree in
principle, but it
> can only break more user files. As long as we're not considering
reorganizing
> the immutable lists, I think it can stay.

Note that there is no necessity of returning a bonafide part of the grob
structure: one can construct a list of properties on the fly. 
Particularly because this is used read-only and only occasionally.

https://codereview.appspot.com/549840044/

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[dak]

On 2020/04/11 16:32:58, thomasmorley651 wrote:
> On 2020/04/11 16:31:48, thomasmorley651 wrote:
> 
> > Btw, it's used in
> > input/regression/multi-measure-rest-reminder.ly
> > input/regression/scheme-text-spanner.ly
> > as well.
> 
> No, that's ly:grob-properties?
> Bad grepping ...

Also icky name overlaps.  ly:grob-properties? was last.  Maybe it should
be ly:context-grob-properties? instead?  It's the thing contexts use for
tracking grob properties.

https://codereview.appspot.com/549840044/

Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[dak]

The description says "[ly:grob-properties] will not return the
properties that were \overridden."

Aren't you confusing this with ly:grob-basic-properties ?  I think
ly:grob-properties will actually _only_ return the properties that were
overriden.

https://codereview.appspot.com/549840044/

Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/549840044/diff/567430043/lily/grob-scheme.cc
File lily/grob-scheme.cc (left):

https://codereview.appspot.com/549840044/diff/567430043/lily/grob-scheme.cc#oldcode327
lily/grob-scheme.cc:327: LY_DEFINE (ly_grob_basic_properties,
"ly:grob-basic-properties",
There is really no point in removing ly:grob-properties while retaining
ly:grob-basic-properties as they are a pair of accessors.

https://codereview.appspot.com/549840044/

Re: Reimplement Scheme_hash_table using linear probing. (issue 559790043 by hanw...@gmail.com)

[dak]

On 2020/04/11 11:36:16, hanwenn wrote:
> On Sat, Apr 11, 2020 at 1:05 PM 
wrote:
> >
> > On 2020/04/11 05:37:39, hanwenn wrote:
> > > >  In addition, I don't think that it is used to a degree where it
> > would
> > > significantly affect LilyPond's performance.
> > >
> > > It is not yet.
> > >
> > > My plan is to plugin this into Grob and Prob and see if there is a
> > measurable
> > > speed improvement. If there is none, it's likely that your double
> > indexing
> > > scheme will also not bring much.
> >
> > I'd strongly suggest we have numbers first before introducing ~300
lines
> > for a custom hash implementation. It's good to have the
implementation
> > available early (I haven't looked at it yet), but I think it should
only
> > be merged with strong evidence that it's worth it.
> 
> I'm not opposed to this, but in the same vein, I think we should then
> hold off on merging David's reorganization of the property accesses in
> https://codereview.appspot.com/573670043/
> 
> -- 
> Han-Wen Nienhuys - mailto:hanw...@gmail.com -
http://www.xs4all.nl/~hanwen

As I already said there: that change is purely syntactically as of that
patch, the difference in appearance is not odious, and it allows for
parallel development of one or more different implementations without
ongoing merge conflicts.

It doesn't favor any particular approach, merely reorganises the macro
call in a manner where more information is available to the
implementation of the macro.

This is not "in the same vein" but more like the aorta.

-- 
David Kastrup


https://codereview.appspot.com/559790043/

Re: Reimplement Scheme_hash_table using linear probing. (issue 559790043 by hanw...@gmail.com)

[dak]

On 2020/04/11 05:37:39, hanwenn wrote:
> >  In addition, I don't think that it is used to a degree where it
would
> significantly affect LilyPond's performance.
> 
> It is not yet. 
> 
> My plan is to plugin this into Grob and Prob and see if there is a
measurable
> speed improvement.

You cannot just "plug it in" since it doesn't integrate seamlessly into
the system of context-managed grob properties (override/revert).  There
is also lily/break-substitution.cc to deal with.

> If there is none, it's likely that your double indexing
> scheme will also not bring much.

I prefer to judge the viability of my approaches based on my own code,
but thanks for worrying.

>
https://codereview.appspot.com/559790043/diff/563830043/input/regression/scheme-unit-test.ly
> File input/regression/scheme-unit-test.ly (right):
> 
>
https://codereview.appspot.com/559790043/diff/563830043/input/regression/scheme-unit-test.ly#newcode6
> input/regression/scheme-unit-test.ly:6: #(ly:test-scheme-hash-table)
> On 2020/04/10 21:43:28, dak wrote:
> > This seems like a rather opaque way of testing functionality.
> 
> how about criticism that is constructive? What do you suggest instead?

Have the actual functionality tested spelled out in Scheme rather than
calling an opaque C++ block?  It's what we do elsewhere.
> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/include/scm-hash.hh
> File lily/include/scm-hash.hh (right):
> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/include/scm-hash.hh#newcode36
> lily/include/scm-hash.hh:36: Entry *table_;
> On 2020/04/10 21:43:28, dak wrote:
> > Any reason this is not a C++ array?  We don't use C style arrays
elsewhere.
> 
> so we can skip the marking for GUILEV2.

So how about making this a regular SCM vector?  That way you can also
skip the individual scm_gc_mark calls in Guilev1 and get everything
initialised to SCM_UNDEFINED.  Another possibility would be to use C++
STL code with a custom allocator.

>
https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc
> File lily/scm-hash.cc (right):
> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode59
> lily/scm-hash.cc:59: if (old_table[i].key != NULL)
> On 2020/04/10 21:43:29, dak wrote:
> > NULL is not a valid SCM value, nor is it guaranteed not to overlap
with valid
> > SCM values that may be stored here.  Better use SCM_UNDEFINED here. 
Also SCM
> > values should not be compared numerically but by using scm_is_eq . 
That
> allows
> > making sure that there are no integer/SCM confusion (there is a
#define one
> can
> > set for that purpose) which are a typical source for problems.
> 
> this would create overhead because scm_gc_calloc allocates data filled
with
> zeroes rather than SCM_xxx. I added some more comment about the 
assumptions to
> the header.

So?  SCM values should not be compared numerically.  If you want to rely
on 0 initialization (which seems rather overblown considering the
overall scheme of things) and not want to break the type system, you
could still do if (SCM_UNPACK (old_table[i].key) != 0) (note that SCM
values are not guaranteed to be equivalent to a pointer type so
comparison to NULL is also a bad idea).

> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode73
> lily/scm-hash.cc:73: uintptr_t x = (uintptr_t) (key);
> On 2020/04/10 21:43:29, dak wrote:
> > For using the numerical value of an SCM, there is SCM_UNPACK. 
Please don't
> use
> > C style casts.  They always succeed, even in cases where they are a
very bad
> > idea.
> 
> Done.
> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode81
> lily/scm-hash.cc:81: {
> On 2020/04/10 21:43:28, dak wrote:
> > This only works for specific cases (uintptr_t == 4 or 8).  Instead
of creating
> > our own implementation, how about submitting a patch to Guile
upstream?  That
> > way there is better review for Guile-specific problems and we don't
have the
> > maintenance cost in our own code.
> 
> no. we'd only get the benefits when we move to GUILE 3.2, whenever
that is
> released.

Guile development works differently.  Stuff that isn't written by Andy
Wingo is placed directly into the "stable" releases if it is considered
suitable.  Otherwise it is getting ignored.  At any rate, there is no
point in parallel development.
> 
>
https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode95
> lily/scm-hash.cc:95: if (table_[i].key != NULL)
> On 2020/04/10 21:43:28, dak wrote:
> > NULL is not a valid SCM value, and SCM values should be compared
using
> scm_is_eq
> > rather than !=.  Usi

Re: Fix line comments for new snippets (issue 549820043 by d...@gnu.org)

[dak]

https://codereview.appspot.com/549820043/diff/565880043/scripts/auxiliar/makelsr.py
File scripts/auxiliar/makelsr.py (right):

https://codereview.appspot.com/549820043/diff/565880043/scripts/auxiliar/makelsr.py#newcode38
scripts/auxiliar/makelsr.py:38: new_lys_marker = "%% generated from %s"
% new_lys
Embarrassingly, this needs to be  as well as I discovered right now
before pushing.  I decided not to restart the review for this.

https://codereview.appspot.com/549820043/

Reimplement Scheme_hash_table using linear probing. (issue 559790043 by hanw...@gmail.com)

[dak]

I don't see the point in reinventing the wheel.  This functionality is
provided both by Guile (where an improved implementation could be
submitted) and C++'s STL.  In addition, I don't think that it is used to
a degree where it would significantly affect LilyPond's performance.

Reworking this to a degree where the quality of its dealing with SCM
values matches that of our code base and documenting its inscrutable
inner workings thus do not seem like they would be worth the effort.


https://codereview.appspot.com/559790043/diff/563830043/input/regression/scheme-unit-test.ly
File input/regression/scheme-unit-test.ly (right):

https://codereview.appspot.com/559790043/diff/563830043/input/regression/scheme-unit-test.ly#newcode6
input/regression/scheme-unit-test.ly:6: #(ly:test-scheme-hash-table)
This seems like a rather opaque way of testing functionality.

https://codereview.appspot.com/559790043/diff/563830043/lily/include/scm-hash.hh
File lily/include/scm-hash.hh (right):

https://codereview.appspot.com/559790043/diff/563830043/lily/include/scm-hash.hh#newcode36
lily/include/scm-hash.hh:36: Entry *table_;
Any reason this is not a C++ array?  We don't use C style arrays
elsewhere.

https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc
File lily/scm-hash.cc (right):

https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode59
lily/scm-hash.cc:59: if (old_table[i].key != NULL)
NULL is not a valid SCM value, nor is it guaranteed not to overlap with
valid SCM values that may be stored here.  Better use SCM_UNDEFINED
here.  Also SCM values should not be compared numerically but by using
scm_is_eq .  That allows making sure that there are no integer/SCM
confusion (there is a #define one can set for that purpose) which are a
typical source for problems.

https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode73
lily/scm-hash.cc:73: uintptr_t x = (uintptr_t) (key);
For using the numerical value of an SCM, there is SCM_UNPACK.  Please
don't use C style casts.  They always succeed, even in cases where they
are a very bad idea.

https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode81
lily/scm-hash.cc:81: {
This only works for specific cases (uintptr_t == 4 or 8).  Instead of
creating our own implementation, how about submitting a patch to Guile
upstream?  That way there is better review for Guile-specific problems
and we don't have the maintenance cost in our own code.

https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode95
lily/scm-hash.cc:95: if (table_[i].key != NULL)
NULL is not a valid SCM value, and SCM values should be compared using
scm_is_eq rather than !=.  Using SCM_UNDEFINED is the proper way of
doing things.

https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode111
lily/scm-hash.cc:111: *idx = int (hash (key) % cap_);
If the capacity is going to be one less than a power of 2, it would
appear to make sense to use a power of 2 instead and use a mask rather
than a modulo operation.  In particular since the hash functions look
like they are designed in a manner where the individual bits are
well-distributed.

https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode161
lily/scm-hash.cc:161: if (j <= gap)
This stuff is completely inscrutable and without any comment.  It would
appear that it may depend on the capacity being one less than a power of
2 (see above) but without any analysis and any comment or justification,
that is quite unclear.

https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode170
lily/scm-hash.cc:170: while (next % cap_ != h);
Fall-through without any assertion making sure that stuff worked right?

https://codereview.appspot.com/559790043/diff/563830043/lily/scm-hash.cc#newcode264
lily/scm-hash.cc:264: class Timestamp
This stuff really has no place in this file.

https://codereview.appspot.com/559790043/

Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)

[dak]

On 2020/04/09 17:07:46, hanwenn wrote:
> I'm curious about these optimizations. Can you say more?

Properties are currently stored in alists.  They can be stored in
vectors instead.  Give all grob properties a number, then have an array
per grob type mapping this number to an index into an array.  The first
number can be memoized for literal property names, turning a property
lookup into two indexing operations.

That's the gist.  For Scheme code, the memoization could be replaced by
macro compilation, but Guilev2 byte compilation of course is sort of a
roadblock here, too.

https://codereview.appspot.com/573670043/

Re: Fix line comments for new snippets (issue 549820043 by d...@gnu.org)

[dak]

Reviewers: lemzwerg,

Message:
On 2020/04/06 12:40:50, lemzwerg wrote:
> LGTM, and please push directly.

I'd rather not have 2.21.0 in a state inconsistent with its makelsr
program.  So if I were to push, I'd need to push the results of a new
makelsr run as well.

Description:
Fix line comments for new snippets

LilyPond line comments should start with "%%" but snippets generated
from Documentation/snippets/new/*.ly previously only used single
percent signs.

Please review this at https://codereview.appspot.com/549820043/

Affected files (+5, -5 lines):
  M scripts/auxiliar/makelsr.py


Index: scripts/auxiliar/makelsr.py
diff --git a/scripts/auxiliar/makelsr.py b/scripts/auxiliar/makelsr.py
index 
3f94a07203a7b8c6391fd24d925bd984a04beed5..73291990b11ecfe4e7c81d6bc8ba43a3eb2c6218
 100755
--- a/scripts/auxiliar/makelsr.py
+++ b/scripts/auxiliar/makelsr.py
@@ -36,12 +36,12 @@ LY_HEADER_LSR = '''%% DO NOT EDIT this file manually; it is 
automatically
 '''
 
 new_lys_marker = "%% generated from %s" % new_lys
-LY_HEADER_NEW = '''%% DO NOT EDIT this file manually; it is automatically
+LY_HEADER_NEW = ''' DO NOT EDIT this file manually; it is automatically
 %s
-%% Make any changes in Documentation/snippets/new/
-%% and then run scripts/auxiliar/makelsr.py
-%%
-%% This file is in the public domain.
+ Make any changes in Documentation/snippets/new/
+ and then run scripts/auxiliar/makelsr.py
+
+ This file is in the public domain.
 ''' % new_lys_marker
 
 options_parser = optparse.OptionParser (

Re: Remove all uses of markup macro from scm/*.scm (issue 575930043 by d...@gnu.org)

[dak]

On 2020/04/03 22:22:31, hanwenn wrote:
> note that this still doesn't fix the next error message for
compilation which is
> 
> 
> 
> fatal error: Not a markup command: line
> 
> which is triggered by the *definition* of the markup macro:
> 
> (defmacro*-public markup (#:rest body)
> ..
>   (car (compile-all-markup-expressions `(#:line ,body
> 
> the markup compilation tries to lookup the 'line' markup dynamically,
and fails.

I propose to leave this new but related piece of crock for the next
issue as it seems a bit more involved and warrants separate analysis,
testing and fixing.

https://codereview.appspot.com/575930043/

Re: output-distance: write HTML as UTF-8 (issue 563810043 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/563810043/diff/545810044/scripts/build/output-distance.py
File scripts/build/output-distance.py (right):

https://codereview.appspot.com/563810043/diff/545810044/scripts/build/output-distance.py#newcode1328
scripts/build/output-distance.py:1328: system (u'echo HEAD is 人人的乐谱软件 >
dir1/tree.gittxt')
This seems sort of weird.  Forgotten test code?

https://codereview.appspot.com/563810043/

output-distance: write HTML as UTF-8 (issue 563810043 by hanw...@gmail.com)

[dak]

Is this likely related to the problems in `make check` that James
currently experiences?

https://codereview.appspot.com/563810043/

Re: mf: use python scripting for generating Emmentaler fonts (issue 553580043 by hanw...@gmail.com)

[dak]

On 2020/04/02 21:09:57, thomasmorley651 wrote:
> On 2020/04/02 21:04:06, dak wrote:
> > On 2020/04/02 20:32:39, hanwenn wrote:
> > > Jonas, what is the status of 2.21.0 ?
> > 
> > I wanted to give Phil the go-ahead tomorrow, once some backdated
convert-ly
> rule
> > is in.  Probably depends on how well the LSR import works whether
this gets
> out
> > then.
> 
> There are two LSR-snippets which will fail with 2.20.0.
> If that's not handled gracefully, I could unapprove them (those two
snippets are
> _not_ doc tagged).
> Should I?

If they are not doc-tagged, they should not be relevant for the LSR
import.  So the time frame of dealing with them is somewhat relaxed. 
None of them is likely relevant with regard to issue 5872, right
(convert-ly rule for converting xxx-yyy-spacing #'whatever = ... to
xxx-yyy-spacing.whatever = ) ?

https://codereview.appspot.com/553580043/

Re: mf: use python scripting for generating Emmentaler fonts (issue 553580043 by hanw...@gmail.com)

[dak]

On 2020/04/02 20:32:39, hanwenn wrote:
> Jonas, what is the status of 2.21.0 ?

I wanted to give Phil the go-ahead tomorrow, once some backdated
convert-ly rule is in.  Probably depends on how well the LSR import
works whether this gets out then.

https://codereview.appspot.com/553580043/

Re: Refactor \markup \pattern (issue 549780045 by d...@gnu.org)

[dak]

Reviewers: hanwenn,


https://codereview.appspot.com/549780045/diff/30045/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

https://codereview.appspot.com/549780045/diff/30045/scm/define-markup-commands.scm#newcode4636
scm/define-markup-commands.scm:4636: (space (if (= axis X) space (* 3.0
space
On 2020/03/31 20:22:35, hanwenn wrote:
> why 3.0 ? add a comment.

vspace has a factor of 3 for its spacing.  Will do.

https://codereview.appspot.com/549780045/diff/30045/scm/define-markup-commands.scm#newcode4675
scm/define-markup-commands.scm:4675: (count (inexact->exact (truncate (/
(- middle-width pattern-width) period
On 2020/03/31 20:22:35, hanwenn wrote:
> unrelated change?

Prerequisite.  The previous version of \pattern was coded in a manner
where it accepted unexact integers which does not make sense.  But our
predicates don't catch that even where warranted and a lot of code will
bomb out on inexact integers late.  Fixing the specific problem here for
now made most sense and would have been required anyway if the
predicates had been made more stringent.  And since it should have been
like this in the first place for obvious reasons, adding a comment here
did not make a lot of sense, either.

Description:
Refactor \markup \pattern

Please review this at https://codereview.appspot.com/549780045/

Affected files (+4, -20 lines):
  M scm/define-markup-commands.scm


Index: scm/define-markup-commands.scm
diff --git a/scm/define-markup-commands.scm b/scm/define-markup-commands.scm
index 
8a9cf25cedeb8470092c6a43d4c62cd6045797e7..9dedd024b4a115c79ecae95a044d5d2f7d40cacb
 100644
--- a/scm/define-markup-commands.scm
+++ b/scm/define-markup-commands.scm
@@ -4615,11 +4615,8 @@ Negative values may be used to produce mirror images.
 ;; Repeating
 
 
-;; TODO: this should really be pieced together at stencil level rather
-;; than markup level
-
 (define-markup-command (pattern layout props count axis space pattern)
-  (integer? integer? number? markup?)
+  (index? index? number? markup?)
   #:category other
   "
 Prints @var{count} times a @var{pattern} markup.
@@ -4636,21 +4633,8 @@ Patterns are distributed on @var{axis}.
 }
 @end lilypond"
   (let* ((pattern-stencil (interpret-markup layout props pattern))
- (pattern-width (interval-length
- (ly:stencil-extent pattern-stencil X)))
- (new-props (prepend-alist-chain 'word-space 0 (prepend-alist-chain 
'baseline-skip 0 props
-(let loop ((i (1- count)) (patterns (markup)))
-  (if (zero? i)
-  (interpret-markup
-   layout
-   new-props
-   (if (= axis X)
-   (markup patterns #:stencil pattern-stencil)
-   (markup #:column (patterns #:stencil pattern-stencil
-  (loop (1- i)
-(if (= axis X)
-(markup patterns #:stencil pattern-stencil #:hspace space)
-(markup #:column (patterns #:stencil pattern-stencil 
#:vspace space
+ (space (if (= axis X) space (* 3.0 space
+(stack-stencils axis 1 space (make-list count pattern-stencil
 
 (define-markup-command (fill-with-pattern layout props space dir pattern left 
right)
   (number? ly:dir? markup? markup? markup?)
@@ -4688,7 +4672,7 @@ Patterns are aligned to the @var{dir} markup.
  (right-width (interval-length (ly:stencil-extent right-stencil X)))
  (middle-width (max 0 (- line-width (+ (+ left-width right-width) (* 
word-space 2)
  (period (+ space pattern-width))
- (count (truncate (/ (- middle-width pattern-width) period)))
+ (count (inexact->exact (truncate (/ (- middle-width pattern-width) 
period
  (x-offset (+ (* (- (- middle-width (* count period)) pattern-width) 
(/ (1+ dir) 2)) (abs (car pattern-x-extent)
 (interpret-markup layout props
   (markup #:stencil left-stencil

Re: Remove all uses of markup macro from scm/*.scm (issue 575930043 by d...@gnu.org)

[dak]

https://codereview.appspot.com/575930043/diff/561620044/scm/time-signature.scm
File scm/time-signature.scm (right):

https://codereview.appspot.com/575930043/diff/561620044/scm/time-signature.scm#newcode23
scm/time-signature.scm:23: (mup (if (procedure? proc)
On 2020/03/30 21:15:15, hanwenn wrote:
> nitpick: how about 'fraction-markup' or 'fraction' ?

Done.

Strictly speaking, the change was likely not necessary in the first
place, but I'd hate keeping (markup ... around in a file, even though it
means something entirely different here.

https://codereview.appspot.com/575930043/diff/561620044/scm/titling.scm
File scm/titling.scm (right):

https://codereview.appspot.com/575930043/diff/561620044/scm/titling.scm#newcode102
scm/titling.scm:102: (mup (ly:output-def-lookup layout what)))
On 2020/03/30 21:15:15, hanwenn wrote:
> title-markup ?
> 

Done.

https://codereview.appspot.com/575930043/

Re: Define Scheme markups using define-public (issue 577720043 by hanw...@gmail.com)

[dak]

On 2020/03/29 14:35:43, hanwenn wrote:
> On Sun, Mar 29, 2020 at 4:12 PM  wrote:
> >
> > On 2020/03/29 13:55:41, hanwenn wrote:
> > > On 2020/03/29 13:52:48, hanwenn wrote:
> > > > retain existing
> > >
> > > how's this? This leaves things backward compatible.
> >
> > Ugly and a maintenance burden since the code is used twice. 
Anything
> > wrong with my proposal?
> 
> I didn't understand your proposal.
> 
> > It does not have duplicate code, makes
> > define-markup-command compilable (while requiring its toplevel use)
and
> > provides a way of doing the same consistently for module-specific
rather
> > than toplevel use.
> >
> > It sacrifices, like your proposal, non-toplevel-performance for the
sake
> > of compilability in Guilev2.  It's just that what the parser then
uses
> > is in a form that could also be used in a reasonably natural manner
from
> > Scheme.
> >
> > Should I write up a patch doing that?
> 
> Yes please.
> 

Working on it.  By the way, it just occured to me that a whole lot of
trouble with byte-compilation is caused by the markup macro.

git grep '(macro\($| \)' scm |wc -l

counts 65 occurences.  Those could certainly be replaced with
make-...-markup functions without causing too much trouble, leaving the
markup macro as only a user-level "feature".  That should significantly
cut down on our problems, right?  The markup macro framework appears to
be one of the worst corners for byte-compilation.

The performance loss due to run-time argument checking instead of
compile-time argument checking (which is likely the motivation for the
markup macro design) is likely insignificant compared to the cost of
actually typesetting markup.

https://codereview.appspot.com/577720043/

Re: Define Scheme markups using define-public (issue 577720043 by hanw...@gmail.com)

[dak]

On 2020/03/29 13:55:41, hanwenn wrote:
> On 2020/03/29 13:52:48, hanwenn wrote:
> > retain existing
> 
> how's this? This leaves things backward compatible.

Ugly and a maintenance burden since the code is used twice.  Anything
wrong with my proposal?  It does not have duplicate code, makes
define-markup-command compilable (while requiring its toplevel use) and
provides a way of doing the same consistently for module-specific rather
than toplevel use.

It sacrifices, like your proposal, non-toplevel-performance for the sake
of compilability in Guilev2.  It's just that what the parser then uses
is in a form that could also be used in a reasonably natural manner from
Scheme.

Should I write up a patch doing that?

https://codereview.appspot.com/577720043/

Re: Define Scheme markups using define-public (issue 577720043 by hanw...@gmail.com)

[dak]

On 2020/03/29 13:55:41, hanwenn wrote:
> On 2020/03/29 13:52:48, hanwenn wrote:
> > retain existing
> 
> how's this? This leaves things backward compatible.
> 
> Note that we're currently incoherent, because you can't do 
> 
>  (let ((n 0)) (define sym val))

"incoherent" means in conflict with oneself.

> so coherency is probably not a great argument for keeping things as
is.

You can do

(define sym (let ((n 0)) val))

however.  Including replacing val inline with a lambda expression making
use of a lexical capture of n.

You can also do
(define sym #f)
(let ((n 0)) (set! sym (lambda () ... n ...))

There is also letrec and various other things, all of which we don't
have available for the somewhat complex semantics of actually defining a
named markup function.

https://codereview.appspot.com/577720043/

Re: Define Scheme markups using define-public (issue 577720043 by hanw...@gmail.com)

[dak]

On 2020/03/29 00:35:02, dak wrote:
> Retaining define-markup-command-internal in order to allow defining
markups in
> LilyPond syntax in a manner that stops being supported from Scheme
seems like
> incoherent design.
> 
> markup-lambdas can be created from within LilyPond using \markup ...
\etc . 
> Having them unusable from Scheme for the purpose of defining a markup
function
> seems inappropriate.
> 
> If unsupporting the functionality via define-markup-command is
desirable (which
> I am not convinced of) or independently, it would seem that a more
> regular/prominent module-define-markup-command! with appropriate
semantics would
> be warranted (and used from the parser) instead of retaining
> define-markup-command-internal as a non-internal of
define-markup-command.

By the way: that would also provide at least a workable way of making
input/regression/pattern-markup-evaluation.ly work again without
requiring a global definition of n .

https://codereview.appspot.com/577720043/

Define Scheme markups using define-public (issue 577720043 by hanw...@gmail.com)

[dak]

Retaining define-markup-command-internal in order to allow defining
markups in LilyPond syntax in a manner that stops being supported from
Scheme seems like incoherent design.

markup-lambdas can be created from within LilyPond using \markup ...
\etc .  Having them unusable from Scheme for the purpose of defining a
markup function seems inappropriate.

If unsupporting the functionality via define-markup-command is desirable
(which I am not convinced of) or independently, it would seem that a
more regular/prominent module-define-markup-command! with appropriate
semantics would be warranted (and used from the parser) instead of
retaining define-markup-command-internal as a non-internal of
define-markup-command.

https://codereview.appspot.com/577720043/

Re: Add dynamic-interface to keepAliveInterfaces (issue 553760043 by j...@abou-samra.fr)

[dak]

What I am worried about is a partitura that puts common dynamics on
every staff, including staves without notes.  That would keep those from
being kept alive.

So the question is whether we should not possibly create a different
keepAliveInterfaces for the Dynamics context?

Opinions?

https://codereview.appspot.com/553760043/

Re: Issue 5862: Prefer astyle 3.1 and update clang-format options (issue 551640043 by nine.fierce.ball...@gmail.com)

[dak]

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.

Our own fixcc script introduces some modifications to naked Astyle if I
remember correctly.  Is there some reasonable hope that we could boil
down a "common denominator" style where alternately applying clang
formatting and fixcc would end up with a fixed end result rather than
some cycle?  Of course, that state would trivially be reached by one
style doing nothing, or the styles focusing on different problems, but
that would not be useful: we'd want to end up with either style doing as
complete of a job as possible so that people using either will commit
stuff that is mostly fine.

https://codereview.appspot.com/551640043/

Re: Remove trailing whitespace {python,scm,lily,scripts}. (issue 547810043 by hanw...@gmail.com)

[dak]

On 2020/03/21 14:17:03, dak wrote:
> On 2020/03/21 13:14:50, hahnjo wrote:
> > Sending to lilypond-devel for broader notice. This is likely to give
> conflicts,
> > maybe we can combine this with running fixcc.py?
> 
> Ah right, that one was still pending anyway.  I think it would take
care of
> trailing spaces in the C++ files.

And the ones in the scm files should likely be combined with running
fixscm.sh .  I think that one warranted changing some block comments not
in Emacs convention, so I hadn't done it on stable yet.

https://codereview.appspot.com/547810043/

Re: Remove trailing whitespace {python,scm,lily,scripts}. (issue 547810043 by hanw...@gmail.com)

[dak]

On 2020/03/21 13:14:50, hahnjo wrote:
> Sending to lilypond-devel for broader notice. This is likely to give
conflicts,
> maybe we can combine this with running fixcc.py?

Ah right, that one was still pending anyway.  I think it would take care
of trailing spaces in the C++ files.

https://codereview.appspot.com/547810043/

Re: aclocal.m4: Support GUILE_CONFIG, document GUILE_FLAVOR (issue 575860044 by d...@gnu.org)

[dak]

https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4
File aclocal.m4 (right):

https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4#newcode625
aclocal.m4:625: AC_ARG_VAR(GUILE_FLAVOR, AS_HELP_STRING([],
On 2020/03/21 05:41:05, lemzwerg wrote:
> What about breaking the line here to get shorter lines?

Acknowledged.

https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4#newcode640
aclocal.m4:640: AC_MSG_ERROR([\$GUILE_CONFIG info guileversion failed:
$tmp_GUILE_FLAVOR])
On 2020/03/21 05:41:05, lemzwerg wrote:
> For consistency I suggest to add `;;` here, too.

Ugh.  Seems like a case where programming style has converged to using
;; as phrase terminator rather than as phrase separator in violation of
the Bourne shell being in the Algol family (and not requiring ";" before
")" either, for example).  Since it specifies fallover behavior into the
next phrase (different fallovers would be ";&" and ";;&"), this is
really ugly.

I'll admit that it is also the practice in this file elsewhere with the
exception of "esac esac".

Will do.

https://codereview.appspot.com/575860044/

Re: aclocal.m4: Support GUILE_CONFIG, document GUILE_FLAVOR (issue 575860044 by d...@gnu.org)

[dak]

On 2020/03/20 14:54:36, hahnjo wrote:
> Generally looks fine to me, you only might want to revisit indention:
aclocal.m4
> uses 4 spaces instead of tabs.

Uh, right.  Done.

https://codereview.appspot.com/575860044/

Re: scripts/build/scan-mf-deps: script to generate MF dependencies (issue 553700043 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/553700043/diff/567370043/mf/invoke-mf2pt.sh
File mf/invoke-mf2pt.sh (right):

https://codereview.appspot.com/553700043/diff/567370043/mf/invoke-mf2pt.sh#newcode4
mf/invoke-mf2pt.sh:4: realpath() {
On 2020/03/17 07:41:25, hahnjo wrote:
> AFAIK sh syntax would be
> realpath()
> (
>   ...
> )
> 
> Curly braces are not part of POSIX
Are you sure?  I have never seen that.  What isn't POSIX is bash's
"function" keyword, but Han-Wen does not use it here.

https://codereview.appspot.com/553700043/

Re: Remove compat hack for Solaris7 and HP-UX (issue 579480047 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/579480047/diff/547750043/aclocal.m4
File aclocal.m4 (right):

https://codereview.appspot.com/579480047/diff/547750043/aclocal.m4#newcode694
aclocal.m4:694: AC_PATH_PROG(BASH, bash, $SHELL)
This sets BASH with a fallback to /bin/sh , meaning that $BASH is not
guaranteed to be anything remotely likely to Bash.

And in the current code base, it would not appear that $BASH is even
getting used anywhere.  At least I cannot find anything suggesting its
use  (rather than its maintenance) with

git grep BASH origin

So what is supposed to be the idea with $BASH here?  What is it supposed
to guarantee providing, and where is it supposed to (eventually?) get
used?

https://codereview.appspot.com/579480047/

Re: scripts/build/scan-mf-deps: script to generate MF dependencies (issue 553700043 by hanw...@gmail.com)

[dak]

Why would we want to move away from GNU make as a build system?

https://codereview.appspot.com/553700043/

Re: Address output-distance problems: (issue 563730043 by hanw...@gmail.com)

[dak]

On 2020/03/12 09:52:31, hahnjo wrote:
> On 2020/03/12 09:33:43, hahnjo wrote:
> > On 2020/03/12 09:22:09, dak wrote:
> > > On 2020/03/12 08:01:03, hahnjo wrote:
> > > > This looks like bash-ism which might explain why it works for
Han-Wen and
> > me.
> > > I
> > > > agree with him that disabling the local-test invocation in
GNUmakefile.in
> is
> > > > probably the easiest solution for now. These tests haven't run
for years,
> so
> > > > we'll definitely be fine without them for a few more days.
> > > 
> > > dak@lola:/usr/local/tmp/lilypond$ dash
> > > $ echo {1,2,3}
> > > {1,2,3}
> > > $ 
> > > 
> > > Ah yes.  Since /bin/sh defaults to dash on Ubuntu (or doesn't it
any more?),
> I
> > > wonder how this escaped testing.
> > 
> > It wasn't tested, that's the point: The initial patch only received
a
> > 'test-baseline' and no 'check' which didn't trigger the python
tests.
> > 'patchy-staging' only runs 'test' as far as I understand, so that
patch landed
> > in master. Now this patch adds it to 'test' which means it's the
first time
> > somebody runs it on Ubuntu.
> > I'm for disabling it again until it receives sufficient testing.
Either in
> > current master removing 'local-check' from
'scripts/build/GNUmakefile' or
> taking
> > the updated patchset from here without the 'local-test' recursion in
> > GNUmakefile.in
> 
> To be clear: I'm not blaming anyone, least of all James;
'test-baseline' really
> was the best way possible to test the initial patch before Han-Wen
added
> compatibility code. IMO this just happens to be a bad coincidence of
two
> problems: The test not running from 'make test', but only 'make
check'; and the
> test not working on Ubuntu at all.

Well, there is not a whole lot to be gained from blaming anybody anyway
when there was no damage (not that the blame game makes a lot of sense
when there is damage).  Patch needs work (whether it contains a problem
itself or triggers a preexisting one that needs to be fixed in order for
the patch to go ahead), but it was caught before the problem affected
everyone.

https://codereview.appspot.com/563730043/

Re: Address output-distance problems: (issue 563730043 by hanw...@gmail.com)

[dak]

On 2020/03/12 08:01:03, hahnjo wrote:
> On 2020/03/11 23:49:23, dak wrote:
> > [...]
> > GNU LilyPond 2.21.0
> > cp: cannot stat '19.sub{-*.signature,.ly,-1.eps,.log,.profile}': No
such file
> or
> > directory
> > test results in  ./out/test-output-distance
> > Traceback (most recent call last):
> >   File "/tmp/lilypond-autobuild/scripts/build/output-distance.py",
line 1561,
> in
> > 
> > main ()
> >   File "/tmp/lilypond-autobuild/scripts/build/output-distance.py",
line 1546,
> in
> > main
> > run_tests ()
> >   File "/tmp/lilypond-autobuild/scripts/build/output-distance.py",
line 1495,
> in
> > run_tests
> > test_compare_tree_pairs ()
> >   File "/tmp/lilypond-autobuild/scripts/build/output-distance.py",
line 1330,
> in
> > test_compare_tree_pairs
> > system ('cp 19.sub{-*.signature,.ly,-1.eps,.log,.profile}
dir1/subdir/')
> >   File "/tmp/lilypond-autobuild/scripts/build/output-distance.py",
line 1304,
> in
> > system
> > assert stat == 0, (stat, x)
> > AssertionError: (256, 'cp
19.sub{-*.signature,.ly,-1.eps,.log,.profile}
> > dir1/subdir/')
> > make[1]: ***
[/tmp/lilypond-autobuild/./scripts/build/GNUmakefile:19:
> > local-test] Error 1
> > make: *** [/tmp/lilypond-autobuild/GNUmakefile.in:328: test] Error 2
> 
> This looks like bash-ism which might explain why it works for Han-Wen
and me. I
> agree with him that disabling the local-test invocation in
GNUmakefile.in is
> probably the easiest solution for now. These tests haven't run for
years, so
> we'll definitely be fine without them for a few more days.

dak@lola:/usr/local/tmp/lilypond$ dash
$ echo {1,2,3}
{1,2,3}
$ 

Ah yes.  Since /bin/sh defaults to dash on Ubuntu (or doesn't it any
more?), I wonder how this escaped testing.



https://codereview.appspot.com/563730043/