Re: Make define-builtin-markup{,-list}-command #:category #:properties keywords (issue160048)

[dak]

http://codereview.appspot.com/160048/diff/1/5
File scm/markup.scm (right):

http://codereview.appspot.com/160048/diff/1/5#newcode74
scm/markup.scm:74: [ :category category ]
On 2009/11/24 11:01:59, Carl wrote:

Does this need to be [ #:category category ] ?


Done.

http://codereview.appspot.com/160048


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: improve doc on markup command writing (issue157133)

[dak]

It would make sense to apply http://codereview.appspot.com/160048>
first and make this patch work on top of that.

There may be cases where a separate property-bind like this is useful in
called routines, and the markup commands from the mentioned patch might
make use of it for implementing #:properties.  However, a separate
property-bind function can't do the job of entering the properties and
their defaults into the command reference.

For this reason, it is probably not a good idea to encourage users to
use a macro like this in markups defined with define-markup-command.
They would need to be rewritten in order to get the properties mentioned
in the command reference.

Since the above-mentioned patch does not provide user-level
documentation, it would make sense to adapt your patch here for
documenting the functionality provided by the above-mentioned patch of
mine.

http://codereview.appspot.com/157133


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Make define-builtin-markup{,-list}-command #:category #:properties keywords (issue160048)

[dak]

On 2009/11/26 09:13:58, nicolas.sceaux wrote:

Hi,



Maybe this patch, as a first step, should focus on the unification of

the syntax

of the two macros?


Hi Nicolas,

patch set 3 already makes make-builtin-markup-command upwards-compatible
with make-markup-command in its current form.  I have not tampered with
make-markup-command at all because I don't want to invest work in an
approach with code duplication.

So I am first trying to get a hang of the problem that the code
duplication and the toplevel module definition are trying to solve.

That's where I would actually have a chance to learn something.

If you feel like taking the work from patch set 3 yourself, go ahead.
But it may be less work to wait and see where I get with my attempts.

All the best.


http://codereview.appspot.com/160048


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Make define-builtin-markup{,-list}-command #:category #:properties keywords (issue160048)

[dak]

Thanks for looking this over.  This patch set is sort of a moving target
due to the removal of markup-init.ly.  For example, recently
documentation about the module related definitions in markup-init.ly has
been added to the programmer's documentation.  If removing
markup-init.ly can be done successfully, this documentation could go as
well.

Unfortunately I still have no feedback about whether the last iteration
of dealing with the "memory leak" situation has been successful for
those who could see the problems.

If that could be corroborrated, investing the time for tracelessly
removing markup-init.ly would be a happier task.


http://codereview.appspot.com/160048/diff/4008/6002
File ly/markup-init.ly (left):

http://codereview.appspot.com/160048/diff/4008/6002#oldcode5
ly/markup-init.ly:5: %%;; to be define later, in a closure
On 2009/12/03 14:58:38, nicolas.sceaux wrote:

If this file is empty, then it shall be deleted


Agreed.

http://codereview.appspot.com/160048/diff/4008/6007
File scm/markup.scm (right):

http://codereview.appspot.com/160048/diff/4008/6007#newcode101
scm/markup.scm:101: (set! body (cddr body)))
On 2009/12/03 14:58:38, nicolas.sceaux wrote:

Why is this needed?
Could #:allow-other-keys take care of that?


defmacro*, in spirit with Common Lisp's keyword arguments, does not
remove keywords from the "rest" argument.  Which is rather stupid, but
not in our hands.  This loop just throws any keyword/value pairs from
the start of the body away without actually looking at them.

This is feasible exactly because #:allow-other-keys has _not_ been set,
so we are sure that all keyword/value pairs we throw away have already
been legitimately parsed.

Do you want a code comment here?  Proposal for the wording?

http://codereview.appspot.com/160048/diff/4008/6007#newcode131
scm/markup.scm:131: ;; Register the new function, for markup
documentation
On 2009/12/03 14:58:38, nicolas.sceaux wrote:

I still have the feeling that user defined commands should not modify

variables

from (lily) module.
Could there be e.g. a module check, so that the macro expand into the
documentation related settings only for builtin commands?


I think it would be more transparent if the documentation selection was
turned off completely by default, and a documentation generating run
explicitly turned it on.

If a user wants to make a small manual for his private markups, being
not allowed to do so because of not being in the lily module would be a
nuisance.  As would be having all lily markups in this manual.

A reasonably easy interface would be to use hashq-set! only
when the respective entity happens to be a hashtable, or even anything
other than #f.

I have not yet done anything in that respect since I have no idea yet
where to properly place the code turning on/off the documentation
collection.

http://codereview.appspot.com/160048/diff/4008/6007#newcode168
scm/markup.scm:168: (set! body (cddr body)))
On 2009/12/03 14:58:38, nicolas.sceaux wrote:

Same as above.


See above.

http://codereview.appspot.com/160048/diff/4008/6007#newcode199
scm/markup.scm:199: (hashq-set! markup-list-functions ,command-name #t)
On 2009/12/03 14:58:38, nicolas.sceaux wrote:

Same as for define-markup-command


See above.

http://codereview.appspot.com/160048


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Make define-builtin-markup{,-list}-command #:category #:properties keywords (issue160048)

[dak]

On 2009/12/03 17:34:24, c_sorensen_byu.edu wrote:


>
> I have not yet done anything in that respect since I have no idea

yet

> where to properly place the code turning on/off the documentation
> collection.
>



How about scm/document-markup.scm?


AFAICS scm/document-markup.scm is only loaded on documentation runs, and
then last.  I don't see how it could be used for switching documentation
collection either on or off.  When it is loaded at all, all information
must already have been collected.

http://codereview.appspot.com/160048


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Make define-builtin-markup{,-list}-command #:category #:properties keywords (issue160048)

[dak]

On 2009/12/03 22:26:04, dak wrote:

On 2009/12/03 17:34:24, c_sorensen_byu.edu wrote:
>
> How about scm/document-markup.scm?



AFAICS scm/document-markup.scm is only loaded on documentation runs,

and then

last.  I don't see how it could be used for switching documentation

collection

either on or off.  When it is loaded at all, all information must

already have

been collected.


Currently I don't see a better way forward than using a special command
line option, something like -dindex-markup on documentation runs.  As
far as I can see, everything else would come too late in the load order.
 Nicolas' proposal to only index inside of the lily module would work,
but it is rather intransparent to the user and I would not want to seal
the possibility to let the user index his own markup, anf just his own.
Since there is no point in the collection overhead on non-documentation
runs, I think it ok if, unlike now, the default is not to collect any
indexing info.

If I get no objections, I'll try making a patch for that.

http://codereview.appspot.com/160048


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Chord repetition: \relative mode (issue164096)

[dak]

On 2009/12/06 17:23:13, nicolas.sceaux wrote:


When dealing with a simple_chord_element (simple notes, rests, etc) or

a

note_chord_element (chord with several notes), the parser updates the

memorized

chord by calling a user-settable memorization function (see

parser.yy).

I think it is the wrong way to go if notes can only be guessed if you
know what user-settable memorization function is in effect.  We should
not sacrifice readability for the sake of saving a few keystrokes.

q should have fixed, simple, understandable semantics even if that means
that it saves fewer keystrokes in border situations.  Having its meaning
change on demand, in particular when this might affect \relative
octaves, is a bad idea.

Write-only music is not a good idea.

http://codereview.appspot.com/164096


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 5740: Add \post to defer context actions to end of time step (issue 581600043 by nine.fierce.ball...@gmail.com)

[dak]

On 2020/02/07 09:19:03, hanwenn wrote:

> David mentions \cadenzaOff in the issue tracker. I think you could fix
the
> behavior inside the Timing_engraver without requiring a new construct
(although,
> if we did this, we'd probably upset bar numbering across existing
scores.)

I think I tried.  At any rate, I was not proposing to change existing
\cadenzaOff functionality since that has clearly defined semantics.  But
it has evaded me to find a way of expressing "end cadenza and bar".  The
best I could do so far could be expressed as

\cadenzaOffAfter =
#(define-music-function (last-note) (ly:music?)
   #{ \partial #(ly:music-duration 1 0 (ly:moment-main (ly:music-length
lastnote)))
  #last-note
   #})

But that's really somewhat inconvenient.

> I hope I am not demoralizing you; by getting this working, you show
mastery of
> advanced LilyPond internals, which is a great achievement!


https://codereview.appspot.com/581600043/

Parse inline scheme using per-expression port (issue 557330043 by hanw...@gmail.com)

[dak]

Ok, that should probably have been stressed somewhat stronger before:
have you checked the preexisting Guilev2 work in branches that Harm
pointed you to?

The problem you are addressing here seems to be the same as the one
tackled in commit c0a8eec034b94be14c7665e2d4c8a94fae111b78 , namely
commit c0a8eec034b94be14c7665e2d4c8a94fae111b78 (origin/dev/guilev2)
Author: David Kastrup 
Date:   Sun Sep 21 18:40:06 2014 +0200

Source_file::init_port: Keep GUILEv2 from redecoding string input

diff --git a/lily/source-file.cc b/lily/source-file.cc
index 5a94927a7f..5ad9c4c6e8 100644
--- a/lily/source-file.cc
+++ b/lily/source-file.cc
@@ -152,7 +152,11 @@ Source_file::init_port ()
   // we do our own utf8 encoding and verification in the parser, so we
   // use the no-conversion equivalent of latin1
   SCM str = scm_from_latin1_string (c_str ());
-  str_port_ = scm_mkstrport (SCM_INUM0, str, SCM_OPN | SCM_RDNG,
__FUNCTION__);
+  scm_dynwind_begin ((scm_t_dynwind_flags)0);
+  // Why doesn't scm_set_port_encoding_x work here?
+  scm_dynwind_fluid (ly_lily_module_constant
("%default-port-encoding"), SCM_BOOL_F);
+  str_port_ = scm_open_input_string (str);
+  scm_dynwind_end ();
   scm_set_port_filename_x (str_port_, ly_string2scm (name_));
 }

Now it is a valid question why this isn't, GUILEV2-guarded, already in
the main code base.  Maybe we should integrate all of the already
existing Guilev2 work into master with priority in order to avoid
duplicate work.

https://codereview.appspot.com/557330043/

Re: Issue 5740: Add \post to defer context actions to end of time step (issue 581600043 by nine.fierce.ball...@gmail.com)

[dak]

On 2020/02/07 15:43:30, Dan Eble wrote:
> On 2020/02/07 09:19:03, hanwenn wrote:
> > On 2020/02/06 14:29:55, Dan Eble wrote:
> > More code means more maintenance liability, so unless
> > it either solves a problem, or it simplifies the existing system, it
would be
> a
> > net negative. 
> 
> You're preaching to the choir.
> 
> > I would really like the problem defined before we try solving it.
> [...]
> > I hope I am not demoralizing you
> 
> It's a good thing you threw in that last part, because from over here
it was
> sounding rather like you resented my posting this for review.  I'll
try to keep
> my reply descriptive.
> 
> I have seen that mailing-list discussions on detailed design--even for
features
> people recognize they need, but especially for those they don't--do
not go far
> or fast.  I suppose it's because few people have the level of
familiarity, the
> current interest, or the time to devote to written communication on
those
> topics.  I'm not blaming them; it's just my diagnosis.  Posting a
review gives
> them something concrete to comment on, and that gets a discussion
going.
> 
> I have the sense that most of my successful contributions have gone
this way. 
> Is this approach as effective as one that might be taken by a
professional
> software development team?  No, but my code is in LilyPond.  Are the
factors
> that make this the most effective approach in this context going to
change?  Not
> likely.
> 
> > I hope I am not demoralizing you
> 
> Well, it's a dilemma: friction either way.  I spent an hour composing
this
> reasoned reply.  What I was hoping for was any of the following kinds
of
> feedback.  Looking past the little lecture on process, I see some of
them
> starting to come through; so, thank you.
> 
> 1. pointers to potential applications (thanks, David et al.)
> 2. The implementation looks sane, but I can't think of a place we
would use this
> currently.  I think you should ping us when your experiments with it
are more
> mature.
> 3. I haven't looked at the implementation, but I can't think of a
place we would
> use this currently.  I think you should ping us when your experiments
are more
> mature. 
> 4. You can approximate this already with X; would that work for you?
> 5. Speaking as a user, I'm not sure where I'd apply this, but calling
it X would
> be more appropriate.
> 6. This is fantastic!  I've been working around not having this for
decades!

Well, the problem is that this triggers some actions at a comparatively
obscure time.  As Han-Wen quite correctly states, that doesn't seem like
something an end-user might need, and as omnipotent developers, if we
needed that kind of functionality in some specific purpose, we could
access it in C++.

But that doesn't mean that it might not solve some task that a
power-user, the growing breed of users working wonders in Scheme, might
solve as part of a larger package.  Would any such task benefit from
clearer or different semantics?  It's really hard to know.

https://codereview.appspot.com/581600043/

Re: Parse inline scheme using per-expression port (issue 557330043 by hanw...@gmail.com)

[dak]

On 2020/02/07 17:10:24, hanwenn wrote:
 
> FWIW, the 2014 commit lacks explanation and comments of what is going
on. I
> would rather see "production quality" code that we submit to master,
so we
> have  a precise record of what is going on, and can make sure the code
> keeps working.

The one I now committed was at a later stage of the whack-a-mole game we
were playing with Guile developers changing the semantics of their
string ports around within the "stable" Guile 2 series.  This commit is
better documented.  The commit I had pointed out to you (sorry for that)
would neither have compiled in current LilyPond, nor worked with Guile
versions later than something like 2.0.9 or so.

https://codereview.appspot.com/557330043/

Re: parser-ly-from-scheme: Make #{ compilable (issue 581610043 by d...@gnu.org)

[dak]

Reviewers: thomasmorley651,


https://codereview.appspot.com/581610043/diff/569310043/scm/parser-ly-from-scheme.scm
File scm/parser-ly-from-scheme.scm (right):

https://codereview.appspot.com/581610043/diff/569310043/scm/parser-ly-from-scheme.scm#newcode19
scm/parser-ly-from-scheme.scm:19: (define (read-lily-expression-internal
lily-string filename line closures)
On 2020/02/08 20:35:49, thomasmorley651 wrote:
> I was going to write that we would need to care about displayLilyMusic
as well,
> though checking again I noticed:
> 
> \void \displayLilyMusic 
> \new Voice { \applyContext #(lambda (ctx) (display ctx)) b4 }
> 
> with an older lily-build against guile-2.0.14 returns:
> 
> \new Voice { \applyContext ##f
>  b4 }
> 
> A todays build with guile-2.2.6 returned:
> 
> \new Voice { \applyContext #(lambda (ctx) (display ctx))
>  b4 }
> 
> Which is nice.
> 
> Not sure whether it's a lilypond or guile improvement.

Uh, is this the right issue to discuss this with?  Also
dak@lola:/usr/local/tmp/lilypond$ dpkg -l guile-2.2-dev
Desired=Unknown/Install/Remove/Purge/Hold
|
Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name   Version  Architecture Description
+++-==---=
ii  guile-2.2-dev  2.2.6+1-1amd64Development files for Guile
2.2

Are you completely sure that you are using a version of LilyPond
compiled with Guile-2.2.6?  Because I did not do that fix in issue 5743
on a whim but because code very similar to the above bombed out in
display-lily-tests.ly.  Maybe the test code in there behaves differently
for some reason?

Description:
parser-ly-from-scheme: Make #{ compilable

For better or worse, Guile 2 has problems byte-compiling function
values.  Factor out the previous internal function
read-lily-expression-internal and refer to it by (module-relative)
name rather than value in Guile 2.

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

Affected files (+11, -11 lines):
  M scm/parser-ly-from-scheme.scm


Index: scm/parser-ly-from-scheme.scm
diff --git a/scm/parser-ly-from-scheme.scm b/scm/parser-ly-from-scheme.scm
index 
2c75010ab809df3e24b3736a6021dad8c6604e59..1bd8ac70da2a6cc2e38f0f513f9ff317af8bab47
 100644
--- a/scm/parser-ly-from-scheme.scm
+++ b/scm/parser-ly-from-scheme.scm
@@ -16,6 +16,13 @@
  You should have received a copy of the GNU General Public License
  along with LilyPond.  If not, see <http://www.gnu.org/licenses/>.
 
+(define (read-lily-expression-internal lily-string filename line closures)
+  (let* ((clone (ly:parser-clone closures (*location*)))
+ (result (ly:parse-string-expression clone lily-string filename line)))
+(if (ly:parser-has-error? clone)
+(ly:parser-error (_ "error in #{ ... #}") (*location*)))
+result))
+
 (define-public (read-lily-expression chr port)
   "Read a lilypond music expression enclosed within @code{#@{} and @code{#@}}
 from @var{port} and return the corresponding Scheme music expression.
@@ -97,16 +104,9 @@ from @var{port} and return the corresponding Scheme music 
expression.
   (else
(do ((c (copy-char) (copy-char)))
((char=? c #\nl)
-
-(define (embedded-lilypond lily-string filename line closures)
-  (let* ((clone (ly:parser-clone closures (*location*)))
- (result (ly:parse-string-expression clone lily-string
- filename line)))
-(if (ly:parser-has-error? clone)
-(ly:parser-error (_ "error in #{ ... #}") (*location*)))
-result))
-(list embedded-lilypond
-  lily-string filename line
-  (cons 'list (reverse! closures)
+(list (if (guile-v2)
+  '(@@ (lily) read-lily-expression-internal)
+  read-lily-expression-internal)
+  lily-string filename line (cons 'list (reverse! closures)
 
 (read-hash-extend #\{ read-lily-expression)

Re: Add Code of Conduct (issue 575620043 by janek.lilyp...@gmail.com)

[dak]

On 2020/02/08 22:57:13, janek wrote:
> Because of significant disagreement, and to ensure that LilyPond
contributors
> don't feel pushed, I am hereby officially withdrawing this proposal. I
apologize
> for the disturbance caused by the way I have introduced this.
> 
> Maybe I'll submit a revised proposal, but if I do, I'll definitely
start with a
> discussion on the mailing list first.

I should apologize for my reaction here.  I need to learn to express "I
don't see how I could do the part required by me to make this work" in a
manner distinguishable from a preventive strike.  A skill that could
have helped in a few situations.

https://codereview.appspot.com/575620043/

Re: Grow heap aggressively during music interpretation (issue 561390043 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/561390043/diff/567180043/lily/include/smobs.hh
File lily/include/smobs.hh (right):

https://codereview.appspot.com/561390043/diff/567180043/lily/include/smobs.hh#newcode312
lily/include/smobs.hh:312: static size_t count;
On 2020/02/09 03:29:26, Dan Eble wrote:
> It seems that this is initialized to zero because it is static, but if
it simply
> had an "= 0", I wouldn't have had to go refresh my memory with a web
search.
> 
> Is it correct that there are no static Smobs anywhere in the program? 

Depends on what you call "Smob".  Protected_scm is a data type
_explicitly_ intended for static variables containing SCM values.  But
it must not contain anything but "immediate" values at program start
time since the memory protection machinery does not start until well
into main.

> If there
> were, it would be responsible to spend some time checking whether this
could be
> affected by the "static initialization order fiasco."  (Maybe you
already have.)

There was something where code was getting run by constructors inserting
into a static STL vector that had its own constructors not yet run. 
Don't remember the details.  I think it was part of the motivation to
create Protected_scm .

https://codereview.appspot.com/561390043/

Re: Doc: Some miscellaneous suggestions from Peter Toye (issue 579280043 by michael.kaepp...@googlemail.com)

[dak]

On 2020/02/09 12:33:23, thomasmorley651 wrote:
> I'd love to see more opinions about comments #12 and #14.
> Otherwise I'd object to this part of the patch

I agree with the objection raised in #12

https://codereview.appspot.com/579280043/

Re: Parse inline scheme using per-expression port (issue 557330043 by hanw...@gmail.com)

[dak]

On 2020/02/09 17:18:19, hanwenn wrote:
> David, please take another look.

First, you are aware that the rationale for

This commit fixes a problem with GUILE 2.2.6, where LilyPond
calculates offsets in the source file as bytes, while GUILE
interprets
the source file as UTF-8 encoded Unicode. As a result, files with
Unicode before embedded scheme break completely.

is fixed in current master?  So we are, due to the late merges into
master of our Guilev2 related work, in the somewhat unhappy situation
that both you and I have worked on the problem and produced different
solutions, you mainly by changing approach and writing a different
function that essentially has the same problem but does not get thrown
permanently out of synch by just restarting every time.  My own solution
basically was obtained by several iterations of pestering the Guile
developers to get something fixed.  It currently is in a form that has
gotten their active blessing.

Now my original solution was aimed at staying in binary even during
Scheme times while Antonio Ospite does go all-in on UTF-8 in Scheme.  So
the result of combining my and his work is a chimæra which I am less
confident about than my own code on its own.  Being in binary mode for
Flex means that we can deal robustly with code that is not properly in
UTF-8.  For example, we had input files that were ASCII-only except for
some comments containing latters in the Latin-1 plane.  In binary mode,
our lexer deals gracefully with such abominations while Guile balks at
such files when doing UTF-8 processing.

I'll take a look at some test cases with either version and see whether
I can trigger obvious problems.  That would not really make an
evaluation of code quality, just give some better idea whether one of
the approaches would warrant further work before getting selected.

https://codereview.appspot.com/557330043/

Re: Fix and align musicxml and input language "deutsch" (issue 547610043 by d...@gnu.org)

[dak]

Reviewers: lemzwerg,

Message:
On 2020/02/09 22:08:11, lemzwerg wrote:
> LGTM
> 
>
https://codereview.appspot.com/547610043/diff/567190043/scm/define-note-names.scm
> File scm/define-note-names.scm (right):
> 
>
https://codereview.appspot.com/547610043/diff/567190043/scm/define-note-names.scm#newcode302
> scm/define-note-names.scm:302: (heh . ,(ly:make-pitch -1 6 SEMI-FLAT))
> What about providing an alias for the old name 'beh' for backward
compatibility?

I'd rather not since it so glaringly wrong.  All other names of kind
"-eh" are _lowered_ by a quarternote, but this one is raised.  Even if
you wanted to start it off b, it would have to be called bih (assuming
quarternote enharmonicity).

Description:
Fix and align musicxml and input language "deutsch"

Has commits:

In \language "deutsch" use b only to replace hes

The quartertone names should all be derived from h instead of b since
beh was being used inconsistently.


Fix deutsch language of musicxml2py

Use b only for hes and nothing else.  Also use asas and asah instead
of ases and aseh.

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

Affected files (+5, -3 lines):
  M python/musicexp.py
  M scm/define-note-names.scm


Index: python/musicexp.py
diff --git a/python/musicexp.py b/python/musicexp.py
index 
c8ff05288deb84f3c39924e78fd92295efff20d7..6daf583f0d834e29ae913afa521da7b0b6e081ce
 100644
--- a/python/musicexp.py
+++ b/python/musicexp.py
@@ -321,11 +321,13 @@ def pitch_nederlands (pitch):
 
 def pitch_english (pitch):
 str = pitch_generic (pitch, ['c', 'd', 'e', 'f', 'g', 'a', 'b'], ['f', 
'qf', 'qs', 's'])
-return str.replace ('aes', 'as').replace ('ees', 'es')
+return str
 
 def pitch_deutsch (pitch):
 str = pitch_generic (pitch, ['c', 'd', 'e', 'f', 'g', 'a', 'h'], ['es', 
'eh', 'ih', 'is'])
-return str.replace ('hes', 'b').replace ('aes', 'as').replace ('ees', 'es')
+if str == 'hes':
+return 'b'
+return str.replace ('aes', 'as').replace ('ase', 'asa').replace ('ees', 
'es')
 
 def pitch_norsk (pitch):
 return pitch_deutsch (pitch)
Index: scm/define-note-names.scm
diff --git a/scm/define-note-names.scm b/scm/define-note-names.scm
index 
461b78b939cade3a69cccaf14cfb21fa74c2e20a..16ecd72489b85addb582c84ae5199f2692f598e7
 100644
--- a/scm/define-note-names.scm
+++ b/scm/define-note-names.scm
@@ -299,7 +299,7 @@
 (heses . ,(ly:make-pitch -1 6 DOUBLE-FLAT))
 (heseh . ,(ly:make-pitch -1 6 THREE-Q-FLAT))
 (b . ,(ly:make-pitch -1 6 FLAT))
-(beh . ,(ly:make-pitch -1 6 SEMI-FLAT))
+(heh . ,(ly:make-pitch -1 6 SEMI-FLAT))
 (h . ,(ly:make-pitch -1 6 NATURAL))
 (hih . ,(ly:make-pitch -1 6 SEMI-SHARP))
 (his . ,(ly:make-pitch -1 6 SHARP))

Re: Grow heap aggressively during music interpretation (issue 561390043 by hanw...@gmail.com)

[dak]

On 2020/02/09 10:08:20, hanwenn wrote:

> A way around that is to change all instances of vectors holding SCM
values to a
> new 
> gc_vector type that has a custom allocator.  That will be significant
work, but
> probably
> desirable in the long term.

I'd recommend a two-pronged approach for now, lobbying for changes in
the bdwgc that make it suitable for dealing with a larger ratio of
finalisers and, while angling for going to vectors with their own
allocators, sticking with code that can do either.  That way bdwgc will
eventually become better for our current work load and if it leads to
faster or more resource-efficient behavior to rely on finalisers for
marking rather than treating everything as conservative pointers, we
still have the option to return to what we are doing, or pick a mixed
strategy (there likely would be no point in ever marking SCM arrays via
mark hooks if we can just allocate them in mark-everything memory areas:
at least that part should never be contentious, and STL structures of
SCM are the most irksome with regard to not causing trouble before
creation or after destruction).

https://codereview.appspot.com/561390043/

Re: Fix and align musicxml and input language "deutsch" (issue 547610043 by d...@gnu.org)

[dak]

On 2020/02/09 22:27:55, lemzwerg wrote:
> > > What about providing an alias for the old name 'beh'
> > for backward compatibility?
> > 
> > I'd rather not since it so glaringly wrong.  [...]
> 
> OK, but then we need either a NEWS entry or a convert-ly rule (or
both) IMHO.

git log -S '\' --pickaxe-regex -- Documentation

does not show any documented German usage.  Language-specific conversion
rules are pretty heavy-handed (basically, a file needs to contain
\language "deutsch" but no other \language to have them apply, and the
string "beh" is also a common element of words like "behalten" so one
needs to prune carefully).  I am comparatively leery assuming that any
preexisting detected use that isn't a false positive in the first place
has worked as intended.  But on the other hand, there was no other way
to enter heh, and it's been around like this for a long time.  I'd
rather have this done proper in 2.20.

https://codereview.appspot.com/547610043/

engraver: continue when trying to create non-existent Grob (issue 547620043 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/547620043/diff/545560043/lily/engraver.cc
File lily/engraver.cc (right):

https://codereview.appspot.com/547620043/diff/545560043/lily/engraver.cc#newcode126
lily/engraver.cc:126: grob = new Item (SCM_EOL);
Ugh.  How is that going to work?  I am not sure we aren't better off
with a hard exit.  Admittedly, a failed assertion makes little sense
after a programming_error: this should have been a hard error in the
first place.

What is the use case?

https://codereview.appspot.com/547620043/

Re: engraver: continue when trying to create non-existent Grob (issue 547620043 by hanw...@gmail.com)

[dak]

On 2020/02/10 21:35:30, hanwenn wrote:
> On 2020/02/10 21:31:46, dak wrote:
> >
https://codereview.appspot.com/547620043/diff/545560043/lily/engraver.cc
> > File lily/engraver.cc (right):
> > 
> >
>
https://codereview.appspot.com/547620043/diff/545560043/lily/engraver.cc#newcode126
> > lily/engraver.cc:126: grob = new Item (SCM_EOL);
> > Ugh.  How is that going to work?  I am not sure we aren't better off
with a
> hard
> > exit.  Admittedly, a failed assertion makes little sense after a
> > programming_error: this should have been a hard error in the first
place.
> > 
> > What is the use case?
> 
> Without this, you get a segfault because we take a cdr of an empty
list.

Ugh.  Ok, that's rather awful.  But don't we run into followup errors
with that make_item (SCM_EOL) ?

https://codereview.appspot.com/547620043/

Re: Doc: Some miscellaneous suggestions from Peter Toye (issue 579280043 by michael.kaepp...@googlemail.com)

[dak]

On 2020/02/10 22:00:04, michael.kaeppler wrote:

> However, I think that the description of LilyPond's internal pitch
data
> structure
> is not helpful for this (pretty introductory) part of the docs.

Agreed.

> The longer I think about it, the more I'm unsure if the term
"alteration" makes
> sense for a basic understanding how pitches are entered in LilyPond.

It's an alteration of the basic note name, not of in-scale notes.

It corresponds to the spelling of the input.

I am not going to meddle with the rest of the discussion.

https://codereview.appspot.com/579280043/

Re: Fix and align musicxml and input language "deutsch" (issue 547610043 by d...@gnu.org)

[dak]

https://codereview.appspot.com/547610043/diff/557380043/python/musicexp.py
File python/musicexp.py (right):

https://codereview.appspot.com/547610043/diff/557380043/python/musicexp.py#newcode324
python/musicexp.py:324: return str
On 2020/02/11 20:39:14, Be-3 wrote:
> Proposal:
> keep 1st character and replace 'fq' by 'tq' in the suffix:
> 
>   return str[:1] + str[1:].replace ('fq', 'tq')

Your proposal does not work for sqs.  Did an independent fix, as I have
not seen your proposal in time.  Please test since this should also
still make it into 2.20.

https://codereview.appspot.com/547610043/

Re: Special-case syntax error of duration before octave marks (issue 557410043 by d...@gnu.org)

[dak]

Reviewers: lemzwerg, Carl,

Message:
On 2020/02/11 22:37:39, Carl wrote:
> On 2020/02/11 21:46:52, lemzwerg wrote:
> > Good idea!  From inspection only, LGTM.
> 
> Sounds like a great idea!
> 
> Carl

Well, I sort of got a feature request. 


Description:
Special-case syntax error of duration before octave marks

A somewhat common note-entry error is to write something like c4''
that is (judging from my own experience) not even unusual for
experienced users to fall victim to occasionally.  So make a special
parser rule that deals "correctly" with that entry and delivers a more
helpful error message than the parser would muster on its own.

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

Affected files (+44, -0 lines):
  M lily/parser.yy


Index: lily/parser.yy
diff --git a/lily/parser.yy b/lily/parser.yy
index 
89511c5d9d67a8091cbf9e043d9b99def124be58..78f0269a55a4cb5999525b2d54c4aa03fdb2ea05
 100644
--- a/lily/parser.yy
+++ b/lily/parser.yy
@@ -3294,6 +3294,11 @@ quotes:
 | sup_quotes
 ;
 
+stray_quotes:
+   sub_quotes
+   | sup_quotes
+   ;
+
 sup_quotes:
'\'' {
$$ = scm_from_int (1);
@@ -3661,6 +3666,45 @@ pitch_or_music:
$$ = n->unprotect ();
}
} %prec ':'
+   // Next rule is a frequent note entry error, like c4''
+   //
+   // It is quite unlikely that an octave check precedes a
+   // duration, but we have to keep it in the rule in order not
+   // to force the parser into early decisions before actually
+   // seeing a stray quote.  So we try to best interpret that
+   // case as well, even though it's not a likely error case.
+   | pitch exclamations questions octave_check duration stray_quotes 
optional_rest post_events {
+   if (!parser->lexer_->is_note_state ())
+   parser->parser_error (@1, _ ("have to be in Note mode 
for notes"));
+   {
+   Music *n = 0;
+   if (scm_is_true ($7))
+   n = MY_MAKE_MUSIC ("RestEvent", @$);
+   else
+   n = MY_MAKE_MUSIC ("NoteEvent", @$);
+
+   if (scm_is_number ($4))
+   {
+   int q = scm_to_int ($4) + scm_to_int ($6);
+   n->set_property ("absolute-octave", 
scm_from_int (q-1));
+   } else
+   $1 = unsmob ($1)->transposed
+   (Pitch (scm_to_int ($6), 
0)).smobbed_copy ();
+
+   n->set_property ("pitch", $1);
+   n->set_property ("duration", $5);
+
+   if (to_boolean ($3))
+   n->set_property ("cautionary", SCM_BOOL_T);
+   if (to_boolean ($2) || to_boolean ($3))
+   n->set_property ("force-accidental", 
SCM_BOOL_T);
+   if (scm_is_pair ($8))
+   n->set_property ("articulations",
+scm_reverse_x ($8, SCM_EOL));
+   $$ = n->unprotect ();
+   }
+   parser->parser_error (@6, _ ("octave marks must precede 
duration"));
+   } %prec ':'
| new_chord post_events {
if (!parser->lexer_->is_chord_state ())
 parser->parser_error (@1, _ ("have to be in Chord mode 
for chords"));

Re: Special-case syntax error of duration before octave marks (issue 557410043 by d...@gnu.org)

[dak]

https://codereview.appspot.com/557410043/diff/545570043/lily/parser.yy
File lily/parser.yy (right):

https://codereview.appspot.com/557410043/diff/545570043/lily/parser.yy#newcode3676
lily/parser.yy:3676: | pitch exclamations questions octave_check
duration stray_quotes optional_rest post_events {
On 2020/02/12 06:45:08, hanwenn wrote:
> why can't this case be folded in to the preceding rule? Maybe this
comment says
> it already; if so an example would help.

Astute observation.  We print out the grammar in the manual and mixing
error productions and normal productions seems weird and misleading. 
However, there is no way to hide error productions from the grammar, so
my version is not better in that regard (though stray_quotes seems
rather obvious).

But you are right: this calls for making folding into the previous rule.
Will do.

https://codereview.appspot.com/557410043/diff/545570043/lily/parser.yy#newcode3706
lily/parser.yy:3706: parser->parser_error (@6, _ ("octave marks must
precede duration"));
On 2020/02/12 06:45:08, hanwenn wrote:
> add a comment that we sholudn't drop this error, even though this
works for
> users (I think we don't want that because it makes parsing .ly harder
for
> others.)

Accepting that "syntax" would give me a rash.  I had enough of a problem
giving the compiler knowledge about it.  Having it accept it would make
me very unhappy.

I'll try to put in an appropriate comment.

https://codereview.appspot.com/557410043/

Move include guard for fluid.hh (issue 565640043 by hanw...@gmail.com)

[dak]

Guess a full test cycle for this one would be excessive.  As far as I am
concerned, feel free to push this one.

https://codereview.appspot.com/565640043/

Remove unused ly_hash_scm (issue 569340043 by hanw...@gmail.com)

[dak]

Issue title is wrong.  Function is ly_scm_hash, not ly_hash_scm.  Also
its prototype in lily/include/lily-guile.hh should get removed.

https://codereview.appspot.com/569340043/

Re: Special-case syntax error of duration before octave marks (issue 557410043 by d...@gnu.org)

[dak]

On 2020/02/13 13:37:52, hanwenn wrote:
> looks OK.
> 
> Is there a way to exercise this? Maybe with #{ c4'' #} or something?

Just { c4'' } would do it, but regtesting error messages is pretty icky.
 Warnings are more straightforward, but I really want an error.

https://codereview.appspot.com/557410043/

Re: Don't count terminating \0 in Source_file::length (issue 579310043 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/579310043/diff/549550043/lily/source-file.cc
File lily/source-file.cc (right):

https://codereview.appspot.com/579310043/diff/549550043/lily/source-file.cc#newcode51
lily/source-file.cc:51: characters_.push_back ((char)c);
Frankly, this seems like C++ should offer something better for reading a
whole input file into a buffer (also it seems like the sort of thing
that would be needed on any non-random access file, not just standard
input).

https://codereview.appspot.com/579310043/

Re: Don't count terminating \0 in Source_file::length (issue 579310043 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/579310043/diff/561450043/lily/source-file.cc
File lily/source-file.cc (right):

https://codereview.appspot.com/579310043/diff/561450043/lily/source-file.cc#newcode137
lily/source-file.cc:137: data_ = string (&chars[0], chars.size ());
gulp_file_to_string ?

That one apparently also does some kind of logging, so one should check
that using it does not lead to duplications in the log.

https://codereview.appspot.com/579310043/

Re: musicxml2ly: portugues notenames and quarternotes in español (issue 571630043 by d...@gnu.org)

[dak]

Reviewers: hanwenn,


https://codereview.appspot.com/571630043/diff/567240043/python/musicexp.py
File python/musicexp.py (right):

https://codereview.appspot.com/571630043/diff/567240043/python/musicexp.py#newcode375
python/musicexp.py:375: "portugues": pitch_portugues,
On 2020/02/15 12:19:54, hanwenn wrote:
> português

I don't think it makes sense to use anything here that doesn't match the
notename language defined in scm/define-note-names.scm .  There is not
even an alias português there.

Description:
musicxml2ly: portugues notenames and quarternotes in español

Given by Torsten Hämmerle after issue 5746 ended.

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

Affected files (+7, -2 lines):
  M python/musicexp.py


Index: python/musicexp.py
diff --git a/python/musicexp.py b/python/musicexp.py
index 
8e3bde379312f570d79ba12b7ec7692eec6764f9..d6d5b6fa56e32cbda1fadc09c51e50ed9035c606
 100644
--- a/python/musicexp.py
+++ b/python/musicexp.py
@@ -348,8 +348,12 @@ def pitch_francais (pitch):
 return str
 
 def pitch_espanol (pitch):
-str = pitch_generic (pitch, ['do', 're', 'mi', 'fa', 'sol', 'la', 'si'], 
['b', None, None, 's'])
-return str
+str = pitch_generic (pitch, ['do', 're', 'mi', 'fa', 'sol', 'la', 'si'], 
['b', 'cb', 'cs', 's'])
+return str.replace ('bc', 'tc').replace ('sc', 'tc')
+
+def pitch_portugues (pitch):
+str = pitch_generic (pitch, ['do', 're', 'mi', 'fa', 'sol', 'la', 'si'], 
['b', 'bqt', 'sqt', 's'])
+return str.replace ('bbqt', 'btqt').replace ('ssqt', 'stqt')
 
 def pitch_vlaams (pitch):
 str = pitch_generic (pitch, ['do', 're', 'mi', 'fa', 'sol', 'la', 'si'], 
['b', None, None, 'k'])
@@ -368,6 +372,7 @@ def set_pitch_language (language):
 "catalan": pitch_catalan,
 "espanol": pitch_espanol,
 "español": pitch_espanol,
+"portugues": pitch_portugues,
 "vlaams": pitch_vlaams}
 pitch_generating_function = function_dict.get (language, pitch_general)
 

Re: Simplify define-markup-[list-]command-internal, (issue 545590045 by hanw...@gmail.com)

[dak]

On 2020/02/16 20:37:34, hanwenn wrote:
> David, this goes on top of https://codereview.appspot.com/555300043/
> 
> maybe I am missing something, but the regtest completes successfully.
Why did
> you add this stuff?

To allow for making aliases etc.  A markup command definition involves a
number of things.  It registers with the markup macro, it creates a
make-...-markup command, it creates a ...-markup definition.  So a bunch
of bindings need to get created.  markup-lambda does not do all that. 
What it does is register the signature.

So you can in effect do
(define-markup-command blah (markup-lambda ...)) (or use the respective
part of an existing markup function or have an autogenerated
markup-lambda definition)
and have all the required bits for putting a markup command into the
name space.

Is there a particular reason you want to see this functionality killed?

https://codereview.appspot.com/545590045/

Re: Simplify define-markup-[list-]command-internal, (issue 545590045 by hanw...@gmail.com)

[dak]

> In this commit, extra support for the case where command-and-args is
empty was added, ie.

That characterisation is completely wrong.  The support is not for the
cases "where command-and-args is empty" but rather where
command-and-args is not a list but a single symbol.  Just like

(define symbol value)
and
(define (symbol arg1 arg2) body...)

are both possible.

https://codereview.appspot.com/545590045/

Re: Generate dependency files with Clang (issue 555300044 by jonas.hahnf...@gmail.com)

[dak]

On 2020/02/16 21:12:50, lemzwerg wrote:
> Good idea, thanks!  LGTM.
> 
> Apropos .lo files: We can remove STEPMAKE_LIBTOOL in `aclocal.m4`
since it isn't
> used.  I think this change would probably fit into this issue, too.

.lo files were likely needed for the midi module used in some Python
programs.  It's not all that long ago that this was replaced:

commit 37893d923c65a5f1aec6c78f7225704d2ccec666
Author: Graham Percival 
Date:   Wed Jan 18 12:09:58 2017 -0800

Remove midi.c

It's also the only thing that we needed a C compiler (rather than C++)
for.  Well, at least regarding native compilation.  GUB of course needs
a C compiler for bootstrapping the cross-building environment.  But that
does not really concern the stuff in the LilyPond repository.

https://codereview.appspot.com/555300044/

Re: Simplify define-markup-[list-]command-internal, (issue 545590045 by hanw...@gmail.com)

[dak]

On 2020/02/16 21:23:56, hanwenn wrote:
> On Sun, Feb 16, 2020 at 9:57 PM  wrote:
> 
> > > In this commit, extra support for the case where command-and-args
is
> > empty was added, ie.
> >
> > That characterisation is completely wrong.  The support is not for
the
> > cases "where command-and-args is empty" but rather where
> > command-and-args is not a list but a single symbol.  Just like
> >
> > (define symbol value)
> > and
> > (define (symbol arg1 arg2) body...)
> >
> 
> I'm trying to get the markup macros working with GUILE 2.x
compilation,
> which means that all calls to module-define! have to go.

No, it means that all calls to module-define! from within a macro have
to go.  Here the macro is already split into the part not trying to
evaluate symbols and argument lists, and a part (the "internal" one)
that is executed at interpretation time where modules are in working
order.  As long as you can specify the _name_ of the internal in a
manner where it is found at interpretation time (since specifying its
_value_ does not work for the byte compiler) you should be fine.  An
example of how to do that is in scm/parser-ly-from-scheme.scm currently:

(list (if (guile-v2)
  '(@@ (lily) read-lily-expression-internal)
  read-lily-expression-internal)
  lily-string filename line (cons 'list (reverse! closures)

It's not really clear how this will play with -dsafe but let's fix stuff
one point at a time.


https://codereview.appspot.com/545590045/

Re: input/regression/multi-measure-rest-reminder: a demo of user-defined grobs (issue 557380044 by hanw...@gmail.com)

[dak]

On 2020/02/17 10:01:29, hanwenn wrote:
>
https://codereview.appspot.com/557380044/diff/569320045/input/regression/multi-measure-rest-reminder.ly
> File input/regression/multi-measure-rest-reminder.ly (right):
> 
>
https://codereview.appspot.com/557380044/diff/569320045/input/regression/multi-measure-rest-reminder.ly#newcode26
> input/regression/multi-measure-rest-reminder.ly:26:
#(set-object-property! 
> 'MultiMeasureRestReminder 'translation-type? ly:grob-properties?)
> On 2020/02/16 13:45:30, thomasmorley651 wrote:
> > To make further overrides possible, add:
> > #(set-object-property! 'MultiMeasureRestReminder 'is-grob? #t)
> > 
> > Would solve the issue in my recent comment
> 
> Feel free to send a change!
> 
> I don't understand why we need is-grob? if the symbol is already
marked with
> translation-type? == ly:grob-properties?  
> 
> David?

Comparatively unrelated things.  At any rate, "the symbol is marked" is
historical baggage and we should instead of using "object-properties"
use an explicit weak hashtable for such things since that would make it
possible to _reset_ the hashtable between user files and thus make sure
that user-defined properties don't bleed from one file to the next.  I
just haven't got around to doing this change yet which will break all
user-created grobs (and some other things implemented in the same
manner) and provide a more official way of extension.

At any rate, is-grob? is the way of checking that something is a grob
name syntactically.  translation-type?, in contrast, is the type of the
context property storing the current grob definition.  Historically,
this was an alist I think, and mixing \set and \override on the same
property consequently could cause fascinating crashes.

Using the alist directly did not allow for reliably overriding and
reverting nested context properties.  People patched around that area
for years, further obfuscating the logical impossibility of getting this
done correctly and getting the errors to become more obscure and trigger
in different circumstances.  So at one point of time, I introduced the
ly:grob-properties? data structure tracking nested overrides separately
and "cooking" the resulting alist only on an as-needed base.  That it is
needed for nothing else is not inherently related to it being tied to a
named grob.

https://codereview.appspot.com/557380044/

Re: Issue 5771: simplify \partial (issue 557440043 by nine.fierce.ball...@gmail.com)

[dak]

https://codereview.appspot.com/557440043/diff/547670043/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

https://codereview.appspot.com/557440043/diff/547670043/ly/music-functions-init.ly#newcode1319
ly/music-functions-init.ly:1319: 'origin (*location*)
On 2020/02/17 16:59:54, Dan Eble wrote:
> I'm curious about when it is important to provide 'origin (*location*)
to
> make-music and when it is not.  Some functions do it and some don't.

define-music-function tacks the origin on the top-level music expression
it returns.  In this case here, that does not help.

Are you sure this is actually a working idea?  At the beginning of
music, Score does not exist and 'Timing is only (reliably?) established
as an alias by the Timing_translator.  For polyrhythmic pieces, the
Timing context alias is moved down the hierarchy along with the
Timing_translator.

https://codereview.appspot.com/557440043/

Re: Issue 5771: simplify \partial (issue 557440043 by nine.fierce.ball...@gmail.com)

[dak]

On 2020/02/17 19:17:04, Dan Eble wrote:
> On 2020/02/17 18:41:58, dak wrote:
> > Are you sure this is actually a working idea?  At the beginning of
music,
> Score
> > does not exist and 'Timing is only (reliably?) established as an
alias by the
> > Timing_translator.  For polyrhythmic pieces, the Timing context
alias is moved
> > down the hierarchy along with the Timing_translator.
> 
> What I'm sure of is that the stated scenario for using
descend-to-context here
> was "the Timing_translator is moved," that
> input/regression/partial-polymetric.ly moves the Timing_translator,
and this
> patch did not make any change in the regression tests.  I won't claim
that there
> is full coverage because I didn't investigate that thoroughly.
> 
> About the alias: I see this in the Score definition in
ly/engraver-init.ly:
> 
>   \alias "Timing"
> 
>   %% An alias for Timing is established by the Timing_translator in
>   %% whatever context it is initialized, and the timing variables are
>   %% then copied from wherever Timing had been previously established.
>   %% The alias at Score level provides a target for initializing
>   %% Timing variables in layout definitions before any
>   %% Timing_translator has been run.
> 
> The only state in which (descend-to-context ... 'Score) should have an
effect is
> when the current context is Global.  In any other state, it should
change
> nothing.  And if the current context is Global, (context-spec-music
... 'Timing)
> should find-or-create the Score because of the alias; descending to
Score first
> should not be necessary.

Well, with a bit of juggling around things I have not been able to
trigger a difference.  I am a bit skeptical that that means nobody else
can: we have inventive users and the startup of Timing has been one area
where combinations of things like \skip, \partial and \time have managed
to create crashes.  Since we have various other context-related changes
slated in 2.21.0, this is one of the cases where I'd likely be happier
if the final push had this change end up in 2.21.1.

https://codereview.appspot.com/557440043/

Re: Implement Grob::event_cause, Grob::ultimate_event_cause (issue 559500043 by d...@gnu.org)

[dak]

Reviewers: hahnjo, hanwenn, Dan Eble,


https://codereview.appspot.com/559500043/diff/559510043/lily/grob.cc
File lily/grob.cc (right):

https://codereview.appspot.com/559500043/diff/559510043/lily/grob.cc#newcode733
lily/grob.cc:733: while (unsmob (cause))
On 2020/02/21 00:13:25, Dan Eble wrote:
> I appreciate that issuing warnings is not performance-sensitive, and
that you
> simply transplanted this code, but what do you think about avoiding
repeating
> the unsmob, to set a good example?  Something like ...
> 
> while (const Grob *g = unsmob (cause))
>   {
> cause = g->get_property ("cause");
>   }

Not going to use const here since it is pointless while we have nothing
like const_unsmob, and would not use that order anyway since I consider
it misleading (it suggests a declaration of a constant, rather than a
pointer to a constant).  Other than that, fine.

Description:
Implement Grob::event_cause, Grob::ultimate_event_cause

Those were implemented in Grob_info only previously which does not make
a lot of sense,
given that they don't access anything particular to Grob_info.


Also contains commit:

Use Grob::event_cause and Grob::ultimate_event_cause where useful



Frankly, this one has been bugging me on and off for years.  I just
had one compilation error too much today.

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

Affected files (+42, -37 lines):
  M lily/accidental-engraver.cc
  M lily/accidental-placement.cc
  M lily/grob.cc
  M lily/grob-info.cc
  M lily/include/grob.hh
  M lily/lyric-engraver.cc
  M lily/percent-repeat-item.cc
  M lily/slur-engraver.cc
  M lily/tie-engraver.cc


Index: lily/accidental-engraver.cc
diff --git a/lily/accidental-engraver.cc b/lily/accidental-engraver.cc
index 
f15981ebb4f3d79f8f16ca073ef1c51935303413..877ec1cc5ba0c21d96fb808ab20d11d19741e06c
 100644
--- a/lily/accidental-engraver.cc
+++ b/lily/accidental-engraver.cc
@@ -374,8 +374,8 @@ Accidental_engraver::stop_translation_timestep ()
 {
   // Don't mark accidentals as "tied" when the pitch is not
   // actually the same.  This is relevant for enharmonic ties.
-  Stream_event *le = unsmob (l->get_property ("cause"));
-  Stream_event *re = unsmob (r->get_property ("cause"));
+  Stream_event *le = l->event_cause ();
+  Stream_event *re = r->event_cause ();
   if (le && re
   && !ly_is_equal (le->get_property ("pitch"), re->get_property 
("pitch")))
 continue;
Index: lily/accidental-placement.cc
diff --git a/lily/accidental-placement.cc b/lily/accidental-placement.cc
index 
7965fcb4b9613399eca830f44a9087aa5709f1ee..e34bfb85293b1c55712d8fd3ac175d2169c2603c
 100644
--- a/lily/accidental-placement.cc
+++ b/lily/accidental-placement.cc
@@ -39,9 +39,8 @@ using std::vector;
 static Pitch *
 accidental_pitch (Grob *acc)
 {
-  SCM cause = acc->get_parent (Y_AXIS)->get_property ("cause");
+  Stream_event *mcause = acc->get_parent (Y_AXIS)->event_cause ();
 
-  Stream_event *mcause = unsmob (cause);
   if (!mcause)
 {
   programming_error ("note head has no event cause");
Index: lily/grob-info.cc
diff --git a/lily/grob-info.cc b/lily/grob-info.cc
index 
70e2ef55b4b966019f8b35e9f437d6637eea6ef0..9b1c265db31ceba86eb00caef323dda4b75319ff
 100644
--- a/lily/grob-info.cc
+++ b/lily/grob-info.cc
@@ -46,8 +46,7 @@ Grob_info::Grob_info ()
 Stream_event *
 Grob_info::event_cause () const
 {
-  SCM cause = grob_->get_property ("cause");
-  return unsmob (cause);
+  return grob_->event_cause ();
 }
 
 Context *
@@ -59,12 +58,7 @@ Grob_info::context () const
 Stream_event *
 Grob_info::ultimate_event_cause () const
 {
-  SCM cause = grob_->self_scm ();
-  while (unsmob (cause))
-{
-  cause = unsmob (cause)->get_property ("cause");
-}
-  return unsmob (cause);
+  return grob_->ultimate_event_cause ();
 }
 
 bool
Index: lily/grob.cc
diff --git a/lily/grob.cc b/lily/grob.cc
index 
a17869b00adf97f330d16ee23c887132899c780c..c4646b96b59d7bd6bb277e20438d19eef14bb126
 100644
--- a/lily/grob.cc
+++ b/lily/grob.cc
@@ -716,20 +716,36 @@ Grob::internal_vertical_less (Grob *g1, Grob *g2, bool 
pure)
   return false;
 }
 
+/
+  CAUSES
+/
+Stream_event *
+Grob::event_cause () const
+{
+  SCM cause = get_property ("cause");
+  return unsmob (cause);
+}
+
+Stream_event *
+Grob::ultimate_event_cause () const
+{
+  SCM cause = self_scm ();
+  while (unsmob (cause))
+{
+  cause = unsmob (cause)->get_property ("cause");
+}
+  return unsmob (cause);
+}
+
+
+
 /
   MESSAGES
 /
 void
 Grob::programming_error (const string &s) const
 {
-  SCM cause = self_scm ();
-  while (Grob *g = unsmob (cause))
-cause = g->get_property ("cause");
-
-  /* ES TODO: ca

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/569390043/diff/551480043/aclocal.m4
File aclocal.m4 (right):

https://codereview.appspot.com/569390043/diff/551480043/aclocal.m4#newcode760
aclocal.m4:760: guile2.2-config guile-2.2-config guile-config-2.2
guile-config2.2 \
I think that at the current point of time, guile1.8-config should be
preferred over guile2.2-config.  The main advantage that Guile2 offers
currently is that it is more likely to be the installed version.  If
anybody goes to the pain these days of actually installing a Guile-1.8
development package, it is pretty safe to assume that it is for the sake
of compiling LilyPond.

Also, I cannot imagine guile-1.9-config to be recommendable for anything
(I mean, it is somewhere between 1.8 and 2.0) and would not look
explicitly for it.

Yes, this is old code, but when we touch it in order to update to
current realities, we should update to current realities.

Do we even have comparative speed and memory use measurements of Guile2
for make all test doc and some large scores?

https://codereview.appspot.com/569390043/diff/551480043/configure.ac
File configure.ac (right):

https://codereview.appspot.com/569390043/diff/551480043/configure.ac#newcode204
configure.ac:204: case "$GUILE_MAJOR_VERSION" in
How is GUILE_MAJOR_VERSION determined?  A cursory read of aclocal.m4 did
not really help.  Guile executable and the Guile development library
will not necessarily be the same version, particularly since more than
one Guile executable can be installed at the same time.

https://codereview.appspot.com/569390043/

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanw...@gmail.com)

[dak]

On 2020/02/21 16:02:14, hahnjo wrote:
> guile-config since 2.0 has the following comment:
> "This script has been deprecated. Just use pkg-config."

It is not something it outputs but rather a script-internal comment.

> Mabye it makes sense to completely turn to pkg-config? My system has $
ll
> /usr/lib/pkgconfig/guile-*
> -rw-r--r-- 1 root root 453 11. Jan 2019 
/usr/lib/pkgconfig/guile-1.8.pc
> -rw-r--r-- 1 root root 849 10. Jan 2019 
/usr/lib/pkgconfig/guile-2.0.pc
> -rw-r--r-- 1 root root 886 10. Jul 2019 
/usr/lib/pkgconfig/guile-2.2.pc
> 
> So this would also work for Guile 1.8 (which should either be
preferred as David
> suggested or completely dropped; not sure if we're already there).

How does it work?  With GUILE_CONFIG, I pass the executable as an
environment variable.  Admittedly, my non-system-wide Guile-1.8 (Ubuntu
no longer has guile-1.8-dev) contains a file
/usr/local/tmp/guile-1.8/lib/pkgconfig/guile-1.8.pc but I don't have an
idea how to use it.

https://codereview.appspot.com/569390043/

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanw...@gmail.com)

[dak]

On 2020/02/21 17:03:07, hanwenn wrote:
> make baseline:
> 
> 1.8:
> real  3m41.714s
> user  6m52.414s
> sys   0m36.662s
> 
> 2.2:
> real  6m8.344s
> user  12m51.799s
> sys   0m34.875s
> 
> I think this warrants some deeper look, though: I have the impression
that the
> lilypond-book regression tests aren't parallelized. They also suffer
from the
> higher start up costs (there are some 60 of them, so that's 90 seconds
right
> there).

I recently found some "could not contact jobserver" in the logs from
make and googling it suggested that this is a consequence of using
$(MAKE) rather than $$(MAKE) inside of macros (causing premature or too
much expansion?).  I have not investigated yet and don't know what
counts as "macro" here.  Or whether this is all nonsense.  Either way,
it should affect Guile-2 just as much as Guile-1.8, shouldn't it?

https://codereview.appspot.com/569390043/

Re: run-and-check: close stdin (issue 545620043 by hanw...@gmail.com)

[dak]

On 2020/02/22 22:08:33, hanwenn wrote:
> On 2020/02/22 22:01:10, lemzwerg wrote:
> > Never seen such code before, but if it fixes an issue... :-)
> 
> it's a cut and paste from the internet,
>
https://blog.apokalyptik.com/2007/10/24/bash-tip-closing-file-descriptors/
> 
> I'm surprised that make passes on the console stdin to child
processes, but it
> does appear to happen.

The default for _any_ program is to pass all open file descriptors it
receives, most certainly the standard file descriptors, on to children. 
The proper fix here of course is not to close stdin but rather to run
pdflatex with -interaction batchmode (where it prints nothing on output
during the run, so you need to consult the log file in case of problems)
or with -interaction nonstopmode where it never stops for input.

https://codereview.appspot.com/545620043/

Re: Use $(MAKE) instead of 'make' in lilypond-book regtests (issue 569400043 by hanw...@gmail.com)

[dak]

On 2020/02/22 23:18:43, hanwenn wrote:
> On 2020/02/22 23:17:50, hanwenn wrote:
> > you were already potentially in a state where you have 3 jobs each
spawning 3
> > lp-book invocations,
> 
> whoops, I mean: 3 lp-book invocations spawning 3 lilypond subprocesses
each, for
> 9 in total.

Considering that I build with CPU_COUNT=9 make -j9 I am pretty sure that
I'd have noticed if that were happening so far.  I may have a nice
number of cores (4) and memory considered plentiful (16GB) for a laptop,
but 81 parallel instances of LilyPond would certainly bring this system
to its knees.

https://codereview.appspot.com/569400043/

Re: Use $(MAKE) instead of 'make' in lilypond-book regtests (issue 569400043 by hanw...@gmail.com)

[dak]

On 2020/02/22 23:56:23, hanwenn wrote:
> On Sun, Feb 23, 2020 at 12:29 AM  wrote:
> >
> > On 2020/02/22 23:18:43, hanwenn wrote:
> > > On 2020/02/22 23:17:50, hanwenn wrote:
> > > > you were already potentially in a state where you have 3 jobs
each
> > spawning 3
> > > > lp-book invocations,
> > >
> > > whoops, I mean: 3 lp-book invocations spawning 3 lilypond
subprocesses
> > each, for
> > > 9 in total.
> >
> > Considering that I build with CPU_COUNT=9 make -j9 I am pretty sure
that
> > I'd have noticed if that were happening so far.  I may have a nice
> > number of cores (4) and memory considered plentiful (16GB) for a
laptop,
> > but 81 parallel instances of LilyPond would certainly bring this
system
> > to its knees.
> 
> In practice, this doesn't happen, because our build system doesn't
> parallelize all that well, and we don't have much large lp-book
> documents anyway

But this patch attempts to make it happen, right?

https://codereview.appspot.com/569400043/

Allow parallelism in input/regression/lilypond-book/ (issue 547680043 by hanw...@gmail.com)

[dak]

Stupid question: does the database design of lilypond-book even allow
for uncoordinated parallel runs?

https://codereview.appspot.com/547680043/

Re: Allow parallelism in input/regression/lilypond-book/ (issue 547680043 by hanw...@gmail.com)

[dak]

On 2020/02/23 10:29:53, hanwenn wrote:
> On 2020/02/23 10:04:46, hanwenn wrote:
> > On 2020/02/23 09:49:24, dak wrote:
> > > Stupid question: does the database design of lilypond-book even
allow for
> > > uncoordinated parallel runs?
> > 
> > It's not a stupid question; it's a good question.  
> > 
> > Writing files atomically (open temp file, write, close, rename),
seems a lot
> of
> > work.
> > 
> > I've considered adding an advisory lock on the DB directory. Since
we max out
> > CPUs when we run lp-book, it shouldn't slow down things, but I was
worried
> about
> > doing file locks cross-platform.
> > 
> > Maybe I can do flock(1), which at least lets limit the worry to OSX
vs. Linux.
> 
> to answer your original question: I think the \sourcefileline
statements can
> differ between snippets written from different lp-book instances, and
this can
> trigger a consistency check failure.

So wouldn't it appear that the way to exploit parallelism with
lilypond-book, short of writing its own jobserver, is to use CPU_COUNT
like we did before?

https://codereview.appspot.com/547680043/

Re: Add a cooperative FS lock to lilypond-book. (issue 555360043 by hanw...@gmail.com)

[dak]

On 2020/02/23 15:54:54, hanwenn wrote:
> I think this is worth it because it simplifies the build system, and
puts the
> locking in the place where we actually access the resource.

Is there any indication that letting Make run multiple instances of
lilypond-book with every instance except one at a time locking up is
going to be a net win for performance?

I still don't see what this is supposed to buy us over using CPU_COUNTS
for invoking parallel LilyPond instances.  In particular since the
parallel LilyPond instances are forked off at a time when LilyPond has
completed its startup, and in the context of the current Guile-v2
integration, startup times are relevant.  Even though considering the
number of files processed in one LilyPond process, their overall impact
should still be comparatively confined.

https://codereview.appspot.com/555360043/

Re: Add a cooperative FS lock to lilypond-book. (issue 555360043 by hanw...@gmail.com)

[dak]

On 2020/02/23 16:23:34, hanwenn wrote:
> On 2020/02/23 16:05:08, dak wrote:
> > On 2020/02/23 15:54:54, hanwenn wrote:
> > > I think this is worth it because it simplifies the build system,
and puts
> the
> > > locking in the place where we actually access the resource.
> > 
> > Is there any indication that letting Make run multiple instances of
> > lilypond-book with every instance except one at a time locking up is
going to
> be
> > a net win for performance?
> 
> input/regression/lilypond-book:
> 
> rm -rf out-tst; time make out=tst local-test -j4 CPU_COUNT=4
> 
> 
> before
> real  1m16.588s
> 
> after
> real  0m25.224s

So the idea is not as much to run parallel instances of lilypond-book
but rather let lilypond-book itself do the serialization.

The net result will be that Make counts lilypond-book's use of 4 CPUs as
just a single CPU, so unless the parallel makes run into a locking
instance of lilypond-book, this will now result in a maximum of 7 jobs
in parallel, right?

https://codereview.appspot.com/555360043/

Re: Add a cooperative FS lock to lilypond-book. (issue 555360043 by hanw...@gmail.com)

[dak]

On 2020/02/23 15:59:14, hahnjo wrote:
> On 2020/02/23 15:54:54, hanwenn wrote:
> > I think this is worth it because it simplifies the build system, and
puts the
> > locking in the place where we actually access the resource.
> 
> Let me disagree: It complicates lilypond-book with something that no
(external)
> user of the script cares about. So IMHO adding brittle locking
requires more
> justification than that.
> 
> > I take your point about dropped files; the best would be fcntl
locks, but I am
> > worried that they might not be supported on windows. Would you know?
> > 
> > Maybe we can just use fcntl locks on unix, and Windows users should
just not
> try
> > to run parallel lp-book invocations.
> 
> Can we please first take a step back and see how much benefit there
actually is?

To be fair, the current situation is that _anybody_ should just not try
to run parallel lp-book invocations, whether from our build system,
started manually from different shells with the same database, or in any
other manner.  The lilybook database is quite a big hack with its main
purpose being speeding up our doc build.  I am not quite sure whether
normal lilypond-book invocations would even use it.  If they do, the
lock might be separately useful to what is going on in our build
process.

https://codereview.appspot.com/555360043/

Re: Issue 5790: Rational conversion clean-up (issue 573570043 by nine.fierce.ball...@gmail.com)

[dak]

On 2020/02/25 08:07:14, hanwenn wrote:
> LGTM
> 
> (I wonder if we should bite the bullet and use uint64_t iso. U64.)

Just for the record: the big bullet would be a Simple_smob wrapper class
around Guile's rational data type.  Showstopper in the current setup:
Moments in data structures contain rationals and would need to get
heap-marked, but the Midi data structures are not memory-managed by
Scheme.  Even the non-showstopper parts would be quite a bit of work. 
Workaround/migration strategy could be a variant adding itself into a
doubly-linked list of data structures to be marked outside of the normal
Scheme mechanisms and handled by construction/destruction and reference
counting, with the obvious memory overhead.

The alternative would be to base our implementation of rationals not on
U64 but on GNU mp.  There is


 -- C Function: void scm_to_mpz (SCM val, mpz_t rop)
 Assign VAL to the multiple precision integer ROP.  VAL must be an
 exact integer, otherwise an error will be signalled.  ROP must have
 been initialized with ‘mpz_init’ before this function is called.
 When ROP is no longer needed the occupied space must be freed with
 ‘mpz_clear’.  *Note (gmp)Initializing Integers::, for details.

 -- C Function: SCM scm_from_mpz (mpz_t val)
 Return the ‘SCM’ value that represents VAL.

but that sounds like slowing down things more.  Maybe we should collect
this JFTR in some actual issue.

https://codereview.appspot.com/573570043/

Re: .dir-locals.el: spell out nil settings (issue 545640044 by d...@gnu.org)

[dak]

Reviewers: lemzwerg,

Message:
On 2020/02/25 11:29:35, lemzwerg wrote:
> LGTM.  Please directly push.

I think the reason I could answer this more or less off-the-cuff was
that it confused the heck out of me until I figured out the reason.  So
my fear is that when people now use M-x add-directory-local-variable RET
for adding more settings, they will think it broken for removing the .
nil stanza.  So I am pretty sure I decided to pay that cost upfront in
the default rather than having users pay it with interest once they dare
using some of the better Emacs features.

It was sort of an on-the-fence decision for me, enough to have your
request tip it the other direction.  But I think it is fair, when
touching this, to give others a chance to weigh in, too.

It's not urgent, is it?

Description:
.dir-locals.el: spell out nil settings

.dir-locals.el establishes our formatting conventions when writing and
formatting stuff with Emacs.  Since it has been written from within
Emacs using M-x add-dir-local-variable RET it spelled
(indent-tabs-mode . nil) as (indent-tabs-mode) which is confusing to
human readers.  So the file has been changed in the repo to the more
explicit form.  While this will not survive further applications of
M-x add-dir-local-variable RET, users are then responsible for their
own differences.

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

Affected files (+3, -3 lines):
  M .dir-locals.el


Index: .dir-locals.el
diff --git a/.dir-locals.el b/.dir-locals.el
index 
69bd6cae380de8a31252ad45f33e78bcaa11dbbe..8ab21b77199db98e1e2654ce5f3b51302f0510ef
 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -3,9 +3,9 @@
 
 ((c++-mode
   (c-file-style . "gnu")
-  (indent-tabs-mode))
+  (indent-tabs-mode . nil))
  (scheme-mode
-  (indent-tabs-mode))
+  (indent-tabs-mode . nil))
  (texinfo-mode
   (fill-column . 66)
-  (indent-tabs-mode)))
+  (indent-tabs-mode . nil)))

Re: Issue 5788: New French Beamimg Approach (issue 557500043 by torsten.haemme...@web.de)

[dak]

On 2020/02/25 13:06:31, Be-3 wrote:
> On 2020/02/24 06:44:39, hanwenn wrote:
> > One thing to consider: since the mechanics are now very predictable,
maybe we
> > can name the property in after its mechanics? ie. french-correction
->
> > stem-end-shorten or something?
> 
> After having thought about it for quite a while I'm not too happy with
> "stem-end-shorten", for the following reasons:
> 
> The term "end" does not carry any information, because stems can only
be
> shortened at their end, as their starting point is nailed to a
notehead. ;)

TabStaff and some rhythmic stems aren't.

> There already are stem-shortenish properties in standard stem
processing
> (beamed-stem-shorten and stem-shorten).
> 
> Most importantly, the french-correction property is different from all
the other
> shortenish propertie in so far as all of them will heavily influence
the layout
> (affecting overall positioning), but "french-correction" is the only
property
> that will shorten the stem for printing only, leaving all positioning
aspects
> untouched.

Then stem-extra-shortening would be the analog to the extra-offset
property that works after positioning.
> Therefore, I'll very much like to have it called something with
"french" in it
> to emphasize the special role it plays in French beaming (and really
*nowhere*
> else).
> 
> Would "french-shorten" be a viable compromise?
> 
> Thanks for looking into this,
> Torsten

It seems weird to give an explicit value here rather than a flag, as the
effect should likely apply regardless of how you scale beams and their
thicknesses up and down.  There is no better way to fudge this
value/calculation in less conspicuously?  Like using
grob::offset-function or one of its cousins/equivalents from
lily/grob-closure.cc ?

https://codereview.appspot.com/557500043/

Re: Issue 5788: New French Beamimg Approach (issue 557500043 by torsten.haemme...@web.de)

[dak]

On 2020/02/25 13:33:13, lemzwerg wrote:

> > > Please add one or more test cases for your 'french-correction'
property.
> >
> > What specific French beaming examples are you missing?
> 
> I was probably unclear.  What I want is an example that shows how the
> 'french-correction' *property* changes the input.  This is (a) to
execute the
> code path, and (b) a visualization of the property change.

It sounds like actual manipulation of that property by the user would
interfere with the implementation of french-beaming.  So maybe just
mark/sort it as an internal property and only regtest french-beaming as
such?

https://codereview.appspot.com/557500043/

Re: Add a cooperative FS lock to lilypond-book. (issue 555360043 by hanw...@gmail.com)

[dak]

On 2020/02/26 08:28:33, hahnjo wrote:
> On 2020/02/26 08:19:39, hahnjo wrote:

> > > On a philosophical level, it is a lilypond-book implementation
detail
> > > that it can't deal with concurrent invocation, so the remediation
for
> > > this problem should be in lilypond-book too.
> > 
> > Let me disagree: It's an implementation detail of make that it runs
things in
> > parallel. IMHO a build system should ensure that the result of
running with
> > multiple jobs is the same as a sequential run.
> 
> That said: I'm also fine if some other developer accepts this patch.
See my
> timing data above to get to your own conclusion. After all, my opinion
is just
> one of a larger range.

My take on this is that this "implementation detail" of parallel
invocation resulting in awkward breakage is something that warrants
fixing irrespective of our build system.  All that the UG states here is

‘--lily-output-dir=DIR’
 Write lily-XXX files to directory DIR, link into ‘--output’
 directory.  Use this option to save building time for documents in
 different directories which share a lot of identical snippets.

It doesn't state at all what happens in cases of contentions.  Fixing
contentions with a lock is a brute-force solution just not allowing for
parallelism, but it is a solution to the contention problem.

It is not a solution to lilypond-book starting more jobs than Make knows
about.  Or to all but one lilypond-book invocation not doing any
progress and blocking Make which could instead start other actual
single-process tasks.  So I see this patch and its approach as an
improvement to lilypond-book.  I don't see that it solves the parallel
build carnage: it just scales down the impact from having to choose
between complete serialization and database failure.

https://codereview.appspot.com/555360043/

Re: Issue 5788: New French Beamimg Approach (issue 557500043 by torsten.haemme...@web.de)

[dak]

https://codereview.appspot.com/557500043/diff/563610046/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

https://codereview.appspot.com/557500043/diff/563610046/scm/define-grob-properties.scm#newcode1374
scm/define-grob-properties.scm:1374: amount of space in case of French
beaming style.")
I'll really make myself unpopular here, so the ones with the original
suggestion implemented here should likely give feedback before anything
is changed again, but:

Now that this is sorted among the _internal_ grob properties for
LilyPond's private use, it seems more appropriate to name it according
to its specific purpose.  My own choice would be
french-beaming-adjustment (if its normal sign is negative which I don't
know) or french-beaming-shortening (if its normal sign is positive). 
I'd have "beaming" in the name because otherwise it could be anything. 
As an internal property, its name does not need to be extra-easy to
type.

Why adjustment instead correction?  Because the default value is not a
badly placed French beaming stem in need of correction.

But the main thing is that I'd like its name to contain "french-beaming"
because it is not intended to be used for anything else and not
guaranteed to work for anything else.

So please: the people arguing for the original name change: just chime
in with "agree", "can find myself able to agree", or "don't agree".

Thanks!

https://codereview.appspot.com/557500043/

Re: Issue 5788: New French Beamimg Approach (issue 557500043 by torsten.haemme...@web.de)

[dak]

On 2020/02/26 21:50:05, lemzwerg wrote:
> > How about
> > * beamed-stem-french-adjustment
> > * french-beaming-stem-adjustment
> > …? 
> 
> I like the second one.

I'd be fine with it.

https://codereview.appspot.com/557500043/

Re: Add a cooperative FS lock to lilypond-book. (issue 555360043 by hanw...@gmail.com)

[dak]

On 2020/02/28 17:57:06, hanwenn wrote:
> On 2020/02/26 11:59:14, dak wrote:

> > It doesn't state at all what happens in cases of contentions. 
Fixing
> > contentions with a lock is a brute-force solution just not allowing
for
> > parallelism, but it is a solution to the contention problem.
> > 
> > It is not a solution to lilypond-book starting more jobs than Make
knows about. 
> > Or to all but one lilypond-book invocation not doing any progress
and blocking
> > Make which could instead start other actual single-process tasks. 
So I see this
> > patch and its approach as an improvement to lilypond-book.  I don't
see that it
> > solves the parallel build carnage: it just scales down the impact
from having to
> > choose between complete serialization and database failure.
> 
> David, I think you are saying this patch is LGTM - could you be
explicit, so
> james understands what is going on?

I think this patch is an improvement over the status quo.  It's sort of
a crutch that works only on some systems and not on NFS as far as I
understand.  And it doesn't actually work well as a job control measure
in connection with parallel Make.  But it does improve lilypond-book
behavior on some systems.  I think that a restricted form of locking is
better than nothing.  I am incidentally not sure just what kind of file
systems minimal VMs without a file system of their own work with: if
they get an NFS view, this would not even work with Lilydev which would
be bad.  But I don't know how VMs do file systems without a partition of
their own.

https://codereview.appspot.com/555360043/

Re: build cleanups. (issue 547690053 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/547690053/diff/567300043/config.make.in
File config.make.in (right):

https://codereview.appspot.com/547690053/diff/567300043/config.make.in#newcode48
config.make.in:48: GROFF = @GROFF@
These sort of drive-by changes without any mention in commit message or
issue make it a bit harder to review.  I find

stepmake/stepmake/documentation-rules.make: troff -man -Tascii $< |
grotty -b -u -o > $@

which is awkward by not using anything actually tested for. 
Nevertheless, use of grotty probably requires the presence of the groff
package.  The test should likely be better targeted than what we
currently have, though.

https://codereview.appspot.com/547690053/

Re: build cleanups. (issue 547690053 by hanw...@gmail.com)

[dak]

On 2020/02/29 22:13:42, hanwenn wrote:
> added 
> 
> * Remove unused GROFF and LD autoconf vars
> 
> to the commit msg.
> 
> https://codereview.appspot.com/547690053/diff/567300043/config.make.in
> File config.make.in (right):
> 
>
https://codereview.appspot.com/547690053/diff/567300043/config.make.in#newcode48
> config.make.in:48: GROFF = @GROFF@
> On 2020/02/29 22:03:03, dak wrote:
> > These sort of drive-by changes without any mention in commit message
or issue
> > make it a bit harder to review.  I find
> > 
> > stepmake/stepmake/documentation-rules.make: troff -man -Tascii
$< | grotty
> > -b -u -o > $@
> > 
> > which is awkward by not using anything actually tested for. 
Nevertheless, use
> > of grotty probably requires the presence of the groff package.  The
test
> should
> > likely be better targeted than what we currently have, though.
> 
> do we use the .txt anywhere? The docker images (which don't include
groff) don't
> seem to fail for this.

What do you mean with "fail for this"?  Since the results of the test
for groff are not actually used anywhere, removing the test will not
change anything.  Removing the groff package from the docker image, in
contrast, could be problematic.  I really have no clue about the
workings of our build system.

https://codereview.appspot.com/547690053/

Re: Remove unused .1 => .txt rule (issue 557560044 by hanw...@gmail.com)

[dak]

On 2020/02/29 22:37:03, hanwenn wrote:
> tested by running 'make dist'

Maybe move the removal of the GROFF test/flag here?  If there should be
some unexpected breakage at some point of time, that makes it more
likely that it can be handled with a single revert.

https://codereview.appspot.com/557560044/

Re: Remove unused .1 => .txt rule (issue 557560044 by hanw...@gmail.com)

[dak]

On 2020/02/29 23:57:03, dak wrote:
> On 2020/02/29 22:37:03, hanwenn wrote:
> > tested by running 'make dist'
> 
> Maybe move the removal of the GROFF test/flag here?  If there should
be some
> unexpected breakage at some point of time, that makes it more likely
that it can
> be handled with a single revert.

Even though the relation at the current point of time is not really
programmatically enforced.  It's just about putting related removals
into the same place.

https://codereview.appspot.com/557560044/

Re: Make make-autochange function upwards-compatible to 2.18 (issue 567280043 by d...@gnu.org)

[dak]

Reviewers: thomasmorley651,

Message:
On 2020/03/01 10:20:01, thomasmorley651 wrote:
> LGTM

I knew I forgot something.  Let's see whether it's still getting picked
up by Phil.

Description:
Make make-autochange function upwards-compatible to 2.18

This entails putting the ref-pitch argument last and making it an
optional argument.

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

Affected files (+8, -4 lines):
  M ly/music-functions-init.ly
  M scm/autochange.scm


Index: ly/music-functions-init.ly
diff --git a/ly/music-functions-init.ly b/ly/music-functions-init.ly
index 
f3893fab2ffb4062d60f496e63b978195b858c7c..ab1aaf56e6b1c75c23f71e449882ed529533098c
 100644
--- a/ly/music-functions-init.ly
+++ b/ly/music-functions-init.ly
@@ -186,7 +186,7 @@ assertBeamSlope =
 
 autoChange =
 #(define-music-function (pitch clef-1 clef-2 music)
-  ((ly:pitch? (ly:make-pitch 0 0)) (ly:context-mod?)(ly:context-mod?) 
ly:music?)
+  ((ly:pitch?) (ly:context-mod?) (ly:context-mod?) ly:music?)
   (_i "Make voices that switch between staves automatically.  As an option the
 pitch where to switch staves may be specified.  The clefs for the staves are
 optional as well.  Setting clefs  works only for implicitly instantiated
@@ -197,7 +197,7 @@ staves.")
 (clef-2 (or clef-2 #{ \with { \clef "bass" } #})))
 (make-simultaneous-music
  (list
-  (descend-to-context (make-autochange-music pitch music) 'Staff
+  (descend-to-context (make-autochange-music music pitch) 'Staff
   "up" clef-1)
   (context-spec-music (make-skip-music skip) 'Staff
   "up" clef-1)
Index: scm/autochange.scm
diff --git a/scm/autochange.scm b/scm/autochange.scm
index 
0e2e4f11e5ec4f7d6e211aca948ccc533906555c..b79c581dc138313ce1a9ef079f9a901ee58dc6a2
 100644
--- a/scm/autochange.scm
+++ b/scm/autochange.scm
@@ -19,7 +19,11 @@
 
 ;; autochange.scm - fairly related to part combining.
 
-(define-public (make-autochange-music ref-pitch music)
+(define-public (make-autochange-music music . ref-pitch)
+  (define ref-pitch-steps
+(if (and (pair? ref-pitch) (car ref-pitch))
+(ly:pitch-steps (car ref-pitch))
+0))
   (define (generate-split-list change-moment prev-dir event-list acc)
 (if (null? event-list)
 acc
@@ -34,7 +38,7 @@
   #f))
(dir (if pitch
 (sign
-  (- (ly:pitch-steps pitch) (ly:pitch-steps 
ref-pitch)))
+  (- (ly:pitch-steps pitch) ref-pitch-steps))
 0)))
   ;; tail recursive.
   (if (and (not (= dir 0))

Re: Issue 5773: Quarter Tones for all Languages incl. MusicXML import (issue 577520043 by torsten.haemme...@web.de)

[dak]

https://codereview.appspot.com/577520043/diff/555310043/Documentation/es/notation/pitches.itely
File Documentation/es/notation/pitches.itely (right):

https://codereview.appspot.com/577520043/diff/555310043/Documentation/es/notation/pitches.itely#newcode524
Documentation/es/notation/pitches.itely:524: @multitable
{@w{@code{português} o}} {@code{do} @code{re}/@code{re} @code{mi}
@code{fa} @code{sol} @code{la} @code{sib} @code{si}}
Starting the Spanish translation with Portuguese seems like a strange
choice.

https://codereview.appspot.com/577520043/diff/555310043/Documentation/es/notation/pitches.itely#newcode558
Documentation/es/notation/pitches.itely:558: @multitable
{@w{@code{português} o}} {@code{s}/@code{-sharp}}
{@code{f}/@code{-flat}} {@code{ss}/@code{x}/@code{-sharpsharp}}
{@code{ff}/@code{-flatflat}}
I am pretty sure that those aren't the Portuguese note names here.

https://codereview.appspot.com/577520043/

Re: Issue 5773: Quarter Tones for all Languages incl. MusicXML import (issue 577520043 by torsten.haemme...@web.de)

[dak]

On 2020/03/01 21:56:33, Be-3 wrote:
> @multitable column entries are for spacing purposes only.  In this
case, they
> contain the exceptionally long English names (independent of the
document
> language).
> 
>
https://codereview.appspot.com/577520043/diff/555310043/Documentation/es/notation/pitches.itely
> File Documentation/es/notation/pitches.itely (right):
> 
>
https://codereview.appspot.com/577520043/diff/555310043/Documentation/es/notation/pitches.itely#newcode558
> Documentation/es/notation/pitches.itely:558: @multitable
{@w{@code{português}
> o}} {@code{s}/@code{-sharp}} {@code{f}/@code{-flat}}
> {@code{ss}/@code{x}/@code{-sharpsharp}} {@code{ff}/@code{-flatflat}}
> On 2020/03/01 19:54:14, dak wrote:
> > I am pretty sure that those aren't the Portuguese note names here.
> 
> No, definitely not.
> BUT (!) the @multitable line is just for column width spacing, thus
mostly
> containing the column entry meant to determine the minimum column
width.

Oh.  Sorry for the confusion, I'll go on with merging master into
translations.  This one tripped me up.

https://codereview.appspot.com/577520043/

Re: Add a cooperative FS lock to lilypond-book. (issue 555360043 by hanw...@gmail.com)

[dak]

On 2020/02/28 18:14:14, dak wrote:
> On 2020/02/28 17:57:06, hanwenn wrote:
> > On 2020/02/26 11:59:14, dak wrote:
> 
> > > It doesn't state at all what happens in cases of contentions. 
Fixing
> > > contentions with a lock is a brute-force solution just not
allowing for
> > > parallelism, but it is a solution to the contention problem.
> > > 
> > > It is not a solution to lilypond-book starting more jobs than Make
knows
> about. 
> > > Or to all but one lilypond-book invocation not doing any progress
and
> blocking
> > > Make which could instead start other actual single-process tasks. 
So I see
> this
> > > patch and its approach as an improvement to lilypond-book.  I
don't see that
> it
> > > solves the parallel build carnage: it just scales down the impact
from
> having to
> > > choose between complete serialization and database failure.
> > 
> > David, I think you are saying this patch is LGTM - could you be
explicit, so
> > james understands what is going on?
> 
> I think this patch is an improvement over the status quo.  It's sort
of a crutch
> that works only on some systems and not on NFS as far as I understand.
 And it
> doesn't actually work well as a job control measure in connection with
parallel
> Make.  But it does improve lilypond-book behavior on some systems.  I
think that
> a restricted form of locking is better than nothing.  I am
incidentally not sure
> just what kind of file systems minimal VMs without a file system of
their own
> work with: if they get an NFS view, this would not even work with
Lilydev which
> would be bad.  But I don't know how VMs do file systems without a
partition of
> their own.

Sigh.  I just noticed that opposed to the patch title, this does not
just introduce a file lock for lilypond-book but _also_ changes the
build system such that now almost double the number of allocated jobs
get used.  It would be good if different topics weren't conflated into
single issues so that it's easier to discuss what one is actually
dealing with and make decisions based on the respective merits of the
individual parts.

"It doesn't actually work well as a job control measure in connection
with parallel Make" should likely have been an indicator of what I
thought I was talking about.

https://codereview.appspot.com/555360043/

Re: Accept GUILE 2 without extra configure options (issue 569390043 by hanw...@gmail.com)

[dak]

On 2020/02/21 16:34:35, hahnjo wrote:
> On 2020/02/21 16:22:52, lemzwerg wrote:
> > > Mabye it makes sense to completely turn to pkg-config?
> > 
> > Sounds sensible.  In particular, it eases cross-compilation.
> 
> Reading my mind ;-)
> 
> On 2020/02/21 16:25:02, dak wrote:
> > > So this would also work for Guile 1.8 (which should either be
preferred as
> > David
> > > suggested or completely dropped; not sure if we're already there).
> > 
> > How does it work?  With GUILE_CONFIG, I pass the executable as an
environment
> > variable.  Admittedly, my non-system-wide Guile-1.8 (Ubuntu no
longer has
> > guile-1.8-dev) contains a file
> > /usr/local/tmp/guile-1.8/lib/pkgconfig/guile-1.8.pc but I don't have
an idea
> how
> > to use it.
> 
>  $ man pkg-config
> ENVIRONMENT
>  PKG_CONFIG_PATH
>  List of secondary directories where ‘.pc’ files are
looked up.
> 
>  PKG_CONFIG_LIBDIR
>  List of primary directories where ‘.pc’ files are looked
up.
> 
> You'd want PKG_CONFIG_PATH, in your case add
> /usr/local/tmp/guile-1.8/lib/pkgconfig/. Setting PKG_CONFIG_LIBDIR
makes
> pkg-config exclude the system-wide directories like /usr/lib/pkgconfig
(which is
> very convenient for cross-compilation!).

Ok, I've been bitten now by this REALLY REALLY BAD, and there is
absolutely NO documentation to be found ANYWHERE with regard to how this
is supposed to be configured.  ./configure --help does not help, there
is absolutely no mention of anything anywhere.

I finally set
[configure_environment]
GUILE=/usr/bin/guile
GUILE_CONFIG=/usr/local/tmp/guile-1.8/bin/guile-config
PKG_CONFIG_PATH=/usr/local/tmp/guile-1.8/lib/pkgconfig

in my ./lilypond-patchy-config file but how are people supposed to
figure that out?  There is absolutely no useful documentation for
getting there.  And as long as Guile-2.2 crashes frequently with
segfaults when used within LilyPond, it is not really an option to leave
integrators with no discernible option to integrate a version of
Guile-1.8 like Debian does.

https://codereview.appspot.com/569390043/

Re: Fix Windows resource to enable manifest (issue 551580044 by truer...@gmail.com)

[dak]

On 2020/03/08 11:27:52, Be-3 wrote:
> LGTM (in combination with issue 5833).  Many users of Windows will be
quite
> happy about this (myself included).
> 
> Wouldn't this have merited an entry in changes.tely?  After all, UTF-8
filenames
> had never worked in Windows from the beginning, so it'd probably be a
good idea
> to let the users know.
> 
> Thanks,
> Torsten

We don't really document bug fixes as a rule in changes.tely.  It's
pretty long already...

https://codereview.appspot.com/551580044/

Re: Fix Windows resource to enable manifest (issue 551580044 by truer...@gmail.com)

[dak]

On 2020/03/08 12:16:55, thomasmorley651 wrote:
> On 2020/03/08 12:11:43, Be-3 wrote:
> > On 2020/03/08 11:38:29, dak wrote:
> > > We don't really document bug fixes as a rule in changes.tely. 
It's pretty
> > long
> > > already...
> > 
> > Yes, certainly.
> > But I think this is a special case.  Speaking for myself, I'm taking
it for
> > granted that UTF-8 filenames just can't be handled by the Windows
version of
> > LilyPond and so I'm not using them, thus never having a chance to
discover
> that
> > all of a sudden, the problem has vanished.
> > 
> > To me, this feels rather like an enhancement/new feature than a
bugfix, as in
> > "LilyPond for Windows can handle UTF-8 filenames now".
> 
> Although never been a windows-user, I second Torsten in this case.
> 
> > I was just suggesting.

Fine with me.  I am a bit afraid that this kind of change is a bit
fragile and/or dependent on build environments, and once we promise this
in Changes, we may be held to that promise, and personally I am not
involved with Windows to any degree where I could contribute to keeping
that promise.  And I am a defensive file namer myself from times
predating not just UTF-8 but also Latin-1.

But that is more an explanation about why I am thinking up excuses about
this not being put in Changes rather than an explanation about why this
should not be in Changes.  So basically, after reconsideration I think
you are right: that would make a good point for Changes.

https://codereview.appspot.com/551580044/

Re: Calculate download sizes rather than hardcoding them (issue 567340043 by d...@gnu.org)

[dak]

On 2020/03/08 20:25:50, hahnjo wrote:
> A general question that I'm probably just unable to find in the
script: As far
> as I understand the documentation file changes, this will include
download sizes
> for both stable and devel releases. How is this going to work for a
single
> source tree that is either stable or devel?

I have no idea and have not received relevant input for it.  So my own
strategy is to let it go in and see what happens.  There is some info
regarding website building in the CG but it hasn't been touched for a
long time.  Note that the half-way failure mode of this patch is to
leave HTML comments in the pages.  So we get to see which pages get the
comments, and which pages get actual sizes.

We can move from there to decide whether the stable branch needs the
same patches (which is likely) and let ourselves be surprised just when
and why the results may migrate to the web pages.
> 
>
https://codereview.appspot.com/567340043/diff/573610046/scripts/build/fix-docsize.sh
> File scripts/build/fix-docsize.sh (right):
> 
>
https://codereview.appspot.com/567340043/diff/573610046/scripts/build/fix-docsize.sh#newcode68
> scripts/build/fix-docsize.sh:68: sed -i -e "$script" $sourcefiles
> "sed -i" is not portable and doesn't work on FreeBSD and macOS IIRC. I
didn't
> check the other commands in here, but I'm sure for that one.

Well, maybe, but:

git grep 'sed -i'
Documentation/GNUmakefile:  sed -i -e 's/ISOLANG *= *fr/ISOLANG =
$(ISOLANG)/' $(ISOLANG)/GNUmakefile
Documentation/contributor/doc-work.itexi:sed -i -r
's/[0-9a-z]@{40@}/NEW-COMMITTISH/' *.texidoc
scripts/auxiliar/update-patch-version.sh:git grep --name-only $1 | xargs
sed -i -e s/$1/$2/g
stepmake/stepmake/po-targets.make:  sed -i '1,2d'
$(po-outdir)/$(package).po
stepmake/stepmake/po-targets.make:  sed -i -e 's/^\# This file is
distributed.*/$(sed-header)/' $(po-outdir)/$(package).po
stepmake/stepmake/po-targets.make:  sed -i -e 's/^\"Content-Type:
text\/plain.*/$(sed-content)/' $(po-outdir)/$(package).po

At least the stuff in po-targets.make looks like it might get executed
on normal builds.  Only glanced over the files, though.

https://codereview.appspot.com/567340043/

Re: Calculate download sizes rather than hardcoding them (issue 567340043 by d...@gnu.org)

[dak]

On 2020/03/08 20:49:46, hahnjo wrote:
> On 2020/03/08 20:45:46, dak wrote:
> > On 2020/03/08 20:25:50, hahnjo wrote:

> > > "sed -i" is not portable and doesn't work on FreeBSD and macOS
IIRC. I
> didn't
> > > check the other commands in here, but I'm sure for that one.
> > 
> > Well, maybe, but:
> > 
> > git grep 'sed -i'
> > Documentation/GNUmakefile:  sed -i -e 's/ISOLANG *= *fr/ISOLANG
=
> > $(ISOLANG)/' $(ISOLANG)/GNUmakefile
> > Documentation/contributor/doc-work.itexi:sed -i -r
> > 's/[0-9a-z]@{40@}/NEW-COMMITTISH/' *.texidoc
> > scripts/auxiliar/update-patch-version.sh:git grep --name-only $1 |
xargs sed
> -i
> > -e s/$1/$2/g
> > stepmake/stepmake/po-targets.make:  sed -i '1,2d'
> $(po-outdir)/$(package).po
> > stepmake/stepmake/po-targets.make:  sed -i -e 's/^\# This file
is
> > distributed.*/$(sed-header)/' $(po-outdir)/$(package).po
> > stepmake/stepmake/po-targets.make:  sed -i -e
's/^\"Content-Type:
> > text\/plain.*/$(sed-content)/' $(po-outdir)/$(package).po
> > 
> > At least the stuff in po-targets.make looks like it might get
executed on
> normal
> > builds.  Only glanced over the files, though.
> 
> Hm okay, never mind then. As a plain build (no docs) is working on
FreeBSD, I
> guess we're not coming across these ;-)

po files are used for LilyPond proper, so if sed -i would be a problem
on FreeBSD, my hunch that those lines would be executed in the context
of a normal build process would be wrong.  I am not too thrilled with
the prospect of going through temporary files, and if I change this to
use an ed script, I'd likely get murdered.

On the other hand, the temporary file could just be named $@.tmp without
much of a problem.  Right?

https://codereview.appspot.com/567340043/

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/

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: 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: 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]

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: 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: 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: 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: 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: 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: 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/

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: 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/

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 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 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: 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: 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: 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: 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/

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: 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/

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: 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: 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/

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: 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/

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: 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/