date:20200128

Reviewers: lemzwerg,

Message:
On 2020/01/27 10:07:20, lemzwerg wrote:
> For which clang-format version is this format file?  It doesn't work
with 7.0.1,
> for example.  Please add this information to the file.

$ clang-format --version
clang-format version 9.0.0 (Fedora 9.0.0-1.fc31)

can you pinpoint which statements cause problems? We could leave out the
default values for enhanced compatibility.

Description:
Add a tentative .clang-format for LilyPond.

To try it out, run

  clang-format -i lily/include/*[ch] lily/*.cc

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

Affected files (+128, -0 lines):
  A .clang-format

Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)

On 2020/01/28 23:38:24, Dan Eble wrote:
> On 2020/01/28 22:06:33, hanwenn wrote:
> >
> > In general, pointers are preferred in function signatures, so you
can see that
> > the value is mutated in the call site. See also
> >
https://google.github.io/styleguide/cppguide.html#Reference_Arguments
> 
> OK, they're preferred by one or more people at Google.  They're not
preferred by
> me, the people I've worked with, or the creator of C++:
>
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-ptr-ref

I've never been a big fan of Stroustrup, so we are both coherent, at
least :-) 

(see question 9
https://developers.slashdot.org/story/00/02/25/1034222/c-answers-from-bjarne-stroustrup)

> > What would we do in a check? Passing a null input is a programming
error; if
> we
> > don't check, we'll generate a segfault and that seems appropriate
for a
> > programming error.
> 
> Not seeing a satisfying way to handle a null pointer is a hint that
using a
> reference would be better.  You've preferred one of two kinds of
run-time
> errors, but the choice you've dismissed is a compile-time error when
someone
> tries to pass a pointer to a function that requires a reference.  A
compile-time
> error is an improvement over a run-time error, isn't it?
>
> I don't want to spend a great deal time trying to change your mind,
and I'm
> certainly not going to tell a project founder what to do here, but I
would rest
> easier if there were at least a comment on the prototype that the
pointer is not
> allowed to be null.

I reorganized a bit more; I hope you like it better now.

BTW - I don't want to tell a C++ expert how to use the language in
general. If we were in an alternate reality  where we could start from
scratch we could reconsider the decision to not use non-const references
in structs and function arguments. As it is however, any that we have
are most likely errors that we should correct. Check
 
grep --color -nH  -e '&' lily/include/*h|grep -v const

Also, it is rare to check incoming parameters for nullness in
implementation.


https://codereview.appspot.com/577410045/

PATCHES - Countdown for January 29th

2020-01-28 Thread pkx166h


Hello,

Here is the current patch countdown list. The next countdown will be on
January 31st

A quick synopsis of all patches currently in the review process can be
found here:

http://philholmes.net/lilypond/allura/




 Push:

5695 reduce dynamic casting - Dan Eble
https://sourceforge.net/p/testlilyissues/issues/5695
http://codereview.appspot.com/575530107

5694 Dot_configuration maintenance - Dan Eble
https://sourceforge.net/p/testlilyissues/issues/5694
http://codereview.appspot.com/577380046

5693 Doc: Corrected doc string for ly:dimension? - davidsg
https://sourceforge.net/p/testlilyissues/issues/5693
http://codereview.appspot.com/547470049

5689 Refactor enforcement of a single Score context - Dan Eble
https://sourceforge.net/p/testlilyissues/issues/5689
http://codereview.appspot.com/551390046

5688 Announce end of multi-measure-rest - hanwen
https://sourceforge.net/p/testlilyissues/issues/5688
http://codereview.appspot.com/561310045

5687 Remove dead code throughout - hanwen
https://sourceforge.net/p/testlilyissues/issues/5687
http://codereview.appspot.com/547470044

5686 Simplify and speed up uniquify - hanwen
https://sourceforge.net/p/testlilyissues/issues/5686
http://codereview.appspot.com/583390043

5685 Remove outdated comment about resizable hash tables - hanwen
https://sourceforge.net/p/testlilyissues/issues/5685
http://codereview.appspot.com/549460043

5684 Explain dead code for GUILE2 - hanwen
https://sourceforge.net/p/testlilyissues/issues/5684
http://codereview.appspot.com/547470043

5680 Provide --loglevel=PROGRESS for make VERBOSE=1 - hanwen
https://sourceforge.net/p/testlilyissues/issues/5680
http://codereview.appspot.com/575540044

5670 lily: fix some type conversion warnings - hanwen
https://sourceforge.net/p/testlilyissues/issues/5670
http://codereview.appspot.com/557190043


 Countdown:

5704 fixcc.py: change recommended use in --help - David Kastrup
https://sourceforge.net/p/testlilyissues/issues/5704
http://codereview.appspot.com/571470043

5698 int->vsize in various alignment and page-layout methods - Dan Eble
https://sourceforge.net/p/testlilyissues/issues/5698
http://codereview.appspot.com/563420043

5690 Tiny fixes for extractpdfmark - Jonas Hahnfeld
https://sourceforge.net/p/testlilyissues/issues/5690
http://codereview.appspot.com/571420055

5683 GUILE2: generate internals doc in UTF-8 - hanwen
https://sourceforge.net/p/testlilyissues/issues/5683
http://codereview.appspot.com/575540045

5677 Set page breaking properties in the System grob - hanwen
https://sourceforge.net/p/testlilyissues/issues/5677
http://codereview.appspot.com/581470047

5672 Clean up and document include file searching - hanwen
https://sourceforge.net/p/testlilyissues/issues/5672
http://codereview.appspot.com/573400043

5646 Switch to Python 3.x - Jonas Hahnfeld
https://sourceforge.net/p/testlilyissues/issues/5646
http://codereview.appspot.com/545370043


 Review:

5705 int->long in Stem::get_beaming and set_beaming - Dan Eble
https://sourceforge.net/p/testlilyissues/issues/5705
http://codereview.appspot.com/561350044

5703 Run scripts/auxiliar/fixcc.py - David Kastrup
https://sourceforge.net/p/testlilyissues/issues/5703
http://codereview.appspot.com/549480043

5702 Disable C++ exceptions - Jonas Hahnfeld
https://sourceforge.net/p/testlilyissues/issues/5702
http://codereview.appspot.com/571430048

5701 Fix --sloppy option of scripts/auxiliar/fixcc.py - David Kastrup
https://sourceforge.net/p/testlilyissues/issues/5701
http://codereview.appspot.com/577420044

5700 Add a tentative .clang-format for LilyPond. - hanwen
https://sourceforge.net/p/testlilyissues/issues/5700
http://codereview.appspot.com/561340043

5682 Only print out open type font substitution if there was a change - 
hanwen

https://sourceforge.net/p/testlilyissues/issues/5682
http://codereview.appspot.com/577390043

5681 Instrument and improve GC time - hanwen
https://sourceforge.net/p/testlilyissues/issues/5681
http://codereview.appspot.com/573420043


 New:

5707 fix \cueDuring and \quoteDuring display test - Dan Eble
https://sourceforge.net/p/testlilyissues/issues/5707
http://codereview.appspot.com/563430046

5706 Doc: Correct and extend infos about LilyDev setup - xmichael-k
https://sourceforge.net/p/testlilyissues/issues/5706
http://codereview.appspot.com/561360043

5699 LilyPond for GUILE 2.2 - hanwen
https://sourceforge.net/p/testlilyissues/issues/5699
http://codereview.appspot.com/563400065

***


Regards,

James

Re: Announce end of multi-measure-rest (issue 561310045 by hanw...@gmail.com)

2020-01-28 Thread pkx166h


Hello,

On 29/01/2020 05:59, hanw...@gmail.com wrote:

ping?

https://codereview.appspot.com/561310045/

There's no need to do this really. That was kind of the point of the 
countdown method we have.


The devs get (essentially) 5 - 7 days to see 2 emails for all issues 
that need looking at. While some do a LGTM, there was some assumption 
that instead of filling up the reviews with +1/LGTM etc. That as long as 
it passed tests and went through the countdown 
(new/review/countdown/push) without complaint it was taken as read the 
patch was OK.


If a dev wants to keep a patch on review longer then set the Tracker 
back to whatever you need (i.e. put it back from countdown to review) 
and I'll just move it on one again the next time.


It saves polluting the reviews.

James

Re: Issue 5707: fix bug in display of \cueDuring and \quoteDuring (issue 563430046 by nine.fierce.ball...@gmail.com)

2020-01-28 Thread dak

On 2020/01/29 03:08:48, Dan Eble wrote:
> change the test, not the display function

Oh, I am sorry to have been misleading with my comment: I think your
original patch was the right thing to do.  I just remarked that what
display-lily printed would have also worked as input so this was not
really bug rather than an uglier alternative.  But your change of its
output clearly was one for the better, even if the previous output was
only uglier than your fix, not strictly wrong.

I should learn to better keep my inclination to lecture in check when
there is nothing to be fixed.

https://codereview.appspot.com/563430046/

Re: Announce end of multi-measure-rest (issue 561310045 by hanw...@gmail.com)

2020-01-28 Thread rietveldpkx--- via Discussions on LilyPond development

ping?

https://codereview.appspot.com/561310045/

Re: Announce end of multi-measure-rest (issue 561310045 by hanw...@gmail.com)

On 2020/01/24 18:40:56, hanwenn wrote:
> That's for anoter change. I thought that I should do
> https://codereview.appspot.com/553400043/ as a scheme engraver, but
maybe it's
> more something for OLL ?

Whatever - I created
https://sourceforge.net/p/testlilyissues/issues/5708/ so it isn't
forgotten.

https://codereview.appspot.com/561310045/

Re: Issue 5707: fix bug in display of \cueDuring and \quoteDuring (issue 563430046 by nine.fierce.ball...@gmail.com)

On 2020/01/29 00:09:44, Dan Eble wrote:
> > 2: treat warnings as errors in display-lily-tests.ly
> > so that bugs will not be overlooked.
> 
> I don't think I tested this part properly.  I'll come back to it.

It works.

https://codereview.appspot.com/563430046/

Re: Issue 5707: fix bug in display of \cueDuring and \quoteDuring (issue 563430046 by nine.fierce.ball...@gmail.com)

2020-01-28 Thread dak

On 2020/01/29 00:06:14, Dan Eble wrote:

Pretty sure that the trigger was

commit ed86e71eb0f17e93036142938d8c490a3e2fcbea
Author: Valentin Villenave 
Date:   Mon Feb 25 17:24:11 2019 +0100

Use "simple strings" rather than #"hash-prefixed Scheme strings"

Now that the user-exposed syntax hardly requires #-prefixed
string arguments anymore, we can do away with that syntax
through various places as well (particularly the LM, where it may
make things slightly less confusing for newcomers).

This commit leaves LSR-imported snippets untouched for now; it
does remove a no-longer-relevant sentence in doc-work.itexi,
and clarifies a few things in fundamental.itely.

and yes, it's not great that this went undetected for that long.  It's
not that the previous output of display-lily was faulty, it's just that
this big replacement changed expectations.

https://codereview.appspot.com/563430046/

Re: Disable C++ exceptions (issue 571430048 by jonas.hahnf...@gmail.com)

2020-01-28 Thread Hans Åberg

> On 28 Jan 2020, at 23:28, hanw...@gmail.com wrote:
> 
> I don't think there is any chance of ever using exceptions in LilyPond,
> because we can't mix them with the GUILE call stack.

One can combine C++ and Guile exceptions, by converting back and forth between 
them. Even though GCC admits throwing C++ exceptions through a C stack by some 
option, somebody mentioned there are complications with that, and better 
avoided.

Re: Issue 5707: fix bug in display of \cueDuring and \quoteDuring (issue 563430046 by nine.fierce.ball...@gmail.com)

> 2: treat warnings as errors in display-lily-tests.ly
> so that bugs will not be overlooked.

I don't think I tested this part properly.  I'll come back to it.


https://codereview.appspot.com/563430046/

Issue 5707: fix bug in display of \cueDuring and \quoteDuring (issue 563430046 by nine.fierce.ball...@gmail.com)

Reviewers: ,

Description:
https://sourceforge.net/p/testlilyissues/issues/5707/

These tests are failing in master @
2ccac23c0e9f7528e55dfd8d711420b4bc5d7a43.
I assume they have been failing for some time.

1: fix bug in display of \cueDuring and \quoteDuring

I'm not at all certain that I've fixed this the right way.
Should the test case be changed instead?
Is there a third option?

2: treat warnings as errors in display-lily-tests.ly
so that bugs will not be overlooked.



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

Affected files (+3, -2 lines):
  M input/regression/display-lily-tests.ly
  M scm/define-music-display-methods.scm


Index: input/regression/display-lily-tests.ly
diff --git a/input/regression/display-lily-tests.ly 
b/input/regression/display-lily-tests.ly
index 
215f130ea788c38a3e0923172d8ff8290b92df75..044fdde922b18e1ddc3af2fcb05bdb12005b28d1
 100644
--- a/input/regression/display-lily-tests.ly
+++ b/input/regression/display-lily-tests.ly
@@ -22,6 +22,7 @@
(cons input-str result-str
 
 #(read-hash-extend #\[ parse-lily-and-compute-lily-string) %{ ] %}
+#(ly:set-option 'warning-as-error #t)
 
 test =
 #(define-void-function (harmless strings)
Index: scm/define-music-display-methods.scm
diff --git a/scm/define-music-display-methods.scm 
b/scm/define-music-display-methods.scm
index 
7bdb757f34ddcacc91952a0db5df144183ffdcee..3516286c17def21193ce9074a17433e2b2171bb1
 100644
--- a/scm/define-music-display-methods.scm
+++ b/scm/define-music-display-methods.scm
@@ -1078,11 +1078,11 @@ Otherwise, return #f."
quoted-context-id "cue"
quoted-context-type 'CueVoice
element ?music))
-(format #f "\\cueDuring #~s #~a ~a"
+(format #f "\\cueDuring ~s #~a ~a"
 ?quoted-music-name
 ?quoted-voice-direction
 (music->lily-string ?music)))
-  (format #f "\\quoteDuring #~s ~a"
+  (format #f "\\quoteDuring ~s ~a"
   (ly:music-property expr 'quoted-music-name)
   (music->lily-string (ly:music-property expr 'element)

Re: Doc: Correct and extend infos about LilyDev setup (issue 561360043 by michael.kaepp...@googlemail.com)

2020-01-28 Thread michael . kaeppler--- via Discussions on LilyPond development

Good work. Needs some revision.

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi
File Documentation/contributor/quick-start.itexi (right):

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode94
Documentation/contributor/quick-start.itexi:94: The zip archive you
downloaded containes the raw disk image and
containes -> contains

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode152
Documentation/contributor/quick-start.itexi:152: @q{Extended features:
Enable EFI}. Otherwise you won't be
... Enable EFI}; otherwise, ...

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode159
Documentation/contributor/quick-start.itexi:159: interaction with your
mouse pointer on both the host and guest and let
... guest, and let ...

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode188
Documentation/contributor/quick-start.itexi:188: You can change the
password of the @q{dev} user similarly, just call
... similarly; just call ...

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode204
Documentation/contributor/quick-start.itexi:204: At first you will be
prompted for the model of your keyboard.
At first, ...

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode205
Documentation/contributor/quick-start.itexi:205: Press Enter to show
further models. In most cases it will be
In most cases, ...

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode206
Documentation/contributor/quick-start.itexi:206: sufficient to choose
number 71 (Generic, 105 keys). After that
After that, ...

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode207
Documentation/contributor/quick-start.itexi:207: choose your keyboard
layout. Now you can customize the function of
Now, ...
Do you detect a pattern here? :)

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode208
Documentation/contributor/quick-start.itexi:208: your AltGr key.
Normally you will use the default layout settings,
Normally, ...

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode209
Documentation/contributor/quick-start.itexi:209: so take number 1. The
same holds for the question whether you want
... the question of whether ...

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode211
Documentation/contributor/quick-start.itexi:211: At last you will be
asked if you want to configure
At last, ...

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode213
Documentation/contributor/quick-start.itexi:213: Presumably you will not
need this, so you can safely type @q{no}.
Presumably, ...

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode216
Documentation/contributor/quick-start.itexi:216: To setup your system
language(charset, localized messages etc.),
set up

https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi#newcode228
Documentation/contributor/quick-start.itexi:228: Finally you should run
a setup script. If you're on the command line
Finally, ...

https://codereview.appspot.com/561360043/

Doc: Correct and extend infos about LilyDev setup (issue 561360043 by michael.kaepp...@googlemail.com)

Reviewers: ,

Message:
This patch reflects some changes and small fixed that I contributed to
LilyDev and which are now in the new release v2. 
https://github.com/fedelibre/LilyDev/releases

Description:
Doc: Correct and extend infos about LilyDev setup

* Adjust filenames, setup description etc. to
match LilyDev 2
* Remove information which is no longer valid
* Describe setup procedure more in detail

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

Affected files (+114, -62 lines):
  M Documentation/contributor/quick-start.itexi

Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)

On 2020/01/28 22:06:33, hanwenn wrote:
>
> In general, pointers are preferred in function signatures, so you can
see that
> the value is mutated in the call site. See also
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments

OK, they're preferred by one or more people at Google.  They're not
preferred by me, the people I've worked with, or the creator of C++:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-ptr-ref

> What would we do in a check? Passing a null input is a programming
error; if we
> don't check, we'll generate a segfault and that seems appropriate for
a
> programming error.

Not seeing a satisfying way to handle a null pointer is a hint that
using a reference would be better.  You've preferred one of two kinds of
run-time errors, but the choice you've dismissed is a compile-time error
when someone tries to pass a pointer to a function that requires a
reference.  A compile-time error is an improvement over a run-time
error, isn't it?

I don't want to spend a great deal time trying to change your mind, and
I'm certainly not going to tell a project founder what to do here, but I
would rest easier if there were at least a comment on the prototype that
the pointer is not allowed to be null.

https://codereview.appspot.com/577410045/

Re: session macros

Han-Wen Nienhuys  writes:

> On Mon, Jan 27, 2020 at 11:33 AM David Kastrup  wrote:
>> >
>> > When calling the macro twice, we'll define add-session-variable twice,
>> > which seems fishy.
>>
>> I think you got confused here.  A macro is just an ordinary function on
>> its inside, it only differs in its calling conventions during
>> evaluation: instead of evaluating its arguments and calling the function
>> body like with ordinary functions, it is called with unevaluated
>> arguments and the result is then evaluated in its expression.
>>
>> add-session-variable is a local function defined inside of
>> define-session.  The effect is that it is not callable externally, and
>> not modifiable externally.  It is just putting stuff near to where it is
>> being used (it is used as a function in the expansion of the macro).
>>
>> define-session is a macro since its use parallels the use of define ,
>> and the first argument of define is the symbol to be defined which must
>> not be evaluated before define-session is being called.
>
> Thanks, I got it now.  I sent a mail to the guile-devel list to
> inquire what is going on.

Got any comments about macros being sooo yesterday compared to syntax
forms?  Syntax forms actually don't work in LilyPond: there was an
incompatibility because the 1.8 implementation will balk if some symbol
used in syntax forms already has a definition, and we have that.  I
forgot the exact symbol at fault.  I think it was some music function
name.

-- 
David Kastrup

Re: session macros

Han-Wen Nienhuys  writes:

> On Mon, Jan 27, 2020 at 11:33 AM David Kastrup  wrote:
>> >
>> > When calling the macro twice, we'll define add-session-variable twice,
>> > which seems fishy.
>>
>> I think you got confused here.  A macro is just an ordinary function on
>> its inside, it only differs in its calling conventions during
>> evaluation: instead of evaluating its arguments and calling the function
>> body like with ordinary functions, it is called with unevaluated
>> arguments and the result is then evaluated in its expression.
>>
>> add-session-variable is a local function defined inside of
>> define-session.  The effect is that it is not callable externally, and
>> not modifiable externally.  It is just putting stuff near to where it is
>> being used (it is used as a function in the expansion of the macro).
>>
>> define-session is a macro since its use parallels the use of define ,
>> and the first argument of define is the symbol to be defined which must
>> not be evaluated before define-session is being called.
>
> Thanks, I got it now.  I sent a mail to the guile-devel list to
> inquire what is going on.

This pattern of putting the bulk of the actual work done by a macro into
a local function can be found more often in the code base these days
than previously.  It helps reducing the "weird" part of a macro to a
minimum, usually resulting in a single simple quasiquoted form while all
the rest is "normal" code.

-- 
David Kastrup

Re: session macros

2020-01-28 Thread Han-Wen Nienhuys

On Mon, Jan 27, 2020 at 11:33 AM David Kastrup  wrote:
> >
> > When calling the macro twice, we'll define add-session-variable twice,
> > which seems fishy.
>
> I think you got confused here.  A macro is just an ordinary function on
> its inside, it only differs in its calling conventions during
> evaluation: instead of evaluating its arguments and calling the function
> body like with ordinary functions, it is called with unevaluated
> arguments and the result is then evaluated in its expression.
>
> add-session-variable is a local function defined inside of
> define-session.  The effect is that it is not callable externally, and
> not modifiable externally.  It is just putting stuff near to where it is
> being used (it is used as a function in the expansion of the macro).
>
> define-session is a macro since its use parallels the use of define ,
> and the first argument of define is the symbol to be defined which must
> not be evaluated before define-session is being called.

Thanks, I got it now.  I sent a mail to the guile-devel list to
inquire what is going on.

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)

2020-01-28 Thread dak

https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-scm.hh
File lily/include/parse-scm.hh (right):

https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-scm.hh#newcode30
lily/include/parse-scm.hh:30: SCM parse_embedded_scheme (Input *i, bool
safe, Lily_parser *parser);
On 2020/01/28 22:06:32, hanwenn wrote:
> On 2020/01/27 13:39:31, Dan Eble wrote:
> > Changing Input& to Input* is more than cosmetic.  Input& requires an
object,
> but
> > Input* admits a nullptr.  I'm concerned that I don't see that any
checks have
> > been added before the pointer is dereferenced.
> 
> This code stores a reference, which is completely out of style in
LilyPond.

It's pretty untypical, not least because unsmobbing works on pointers. 
That being said, Input is likely the least coherently treated data
structure in that regard if I remember correctly, quite often getting
passed by copy (as seen in the signature of evaluate_embedded_scheme). 
Part of the reason for this use may be that for Scheme protection to set
in, you need to actually smobify the structure and then keep an SCM
value around in the stack somewhere which may prove inconvenient. 
Having a local copy and passing a reference around is quite more static
regarding the life times.

Things go as far as Input() creating a "null input" without location
data, so the value "no input" is not actually represented by using a
null pointer.

All that peculiarity of Input was not invented by me but originally
there.  Part of the reason may be that parser.yy needs a fixed data type
for its @$ and similar location data, so the C structure interface
inherent there might have been partly responsible for this somewhat
peculiar usage.

I don't have a particular opinion on this usage myself but think that
the current use of a reference might have been done by me trying to keep
with the general spirit of how Input is getting used without losing
sight of C++.

Not necessarily successfully so, but I doubt there were a lot of
reviewers advising me at the time.

https://codereview.appspot.com/577410045/

Re: Disable C++ exceptions (issue 571430048 by jonas.hahnf...@gmail.com)

LGTM

I don't think there is any chance of ever using exceptions in LilyPond,
because we can't mix them with the GUILE call stack.

https://codereview.appspot.com/571430048/

Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)

https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-scm.hh
File lily/include/parse-scm.hh (right):

https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-scm.hh#newcode30
lily/include/parse-scm.hh:30: SCM parse_embedded_scheme (Input *i, bool
safe, Lily_parser *parser);
On 2020/01/27 13:39:31, Dan Eble wrote:
> Changing Input& to Input* is more than cosmetic.  Input& requires an
object, but
> Input* admits a nullptr.  I'm concerned that I don't see that any
checks have
> been added before the pointer is dereferenced.

This code stores a reference, which is completely out of style in
LilyPond.

In general, pointers are preferred in function signatures, so you can
see that the value is mutated in the call site. See also
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

What would we do in a check? Passing a null input is a programming
error; if we don't check, we'll generate a segfault and that seems
appropriate for a programming error.

https://codereview.appspot.com/577410045/

Re: automated formatting

2020-01-28 Thread Dan Eble

On Jan 28, 2020, at 12:11, Carl Sorensen  wrote:
>Can you provide me with a presubmit hook that applies fixcc? Can you
>guarantee that, if fixcc has run on the code, there will be no further
>remarks on code formatting during review?
> 
> I think that it's a really good idea to have presubmit hooks.  I believe, but 
> can't guarantee, that if all code were automatically reformatted on 
> submission (by either fixcc or clang-format) there would be virtually no 
> comments on formatting.  And you could completely ignore any that were made.

I'm all for making it possible for a contributor to set up something like this 
if he pleases, but I think it's a bad idea to rely on this level of automation 
and integration with a specific source-control tool.

I don't trust restyling programs to do the most desirable thing for readability 
100% of the time, and I especially don't trust them in certain situations, such 
as when I'm checking in incomplete work because of an urgent need to switch to 
another branch.

Also, our basic goal is that none of the cooks poisons the soup, i.e. merges 
badly formatted code to master.  If some of them want to experiment with wild 
mushrooms on the side, micromanaging their commits doesn't guard the soup any 
better than checking when they present their work for integration.

If you build a style check into "make check", then it will happen whenever 
contributors test their code and it will happen during the review countdown.  
If you also build a style check into the final tests before merging staging to 
master, the goal will be achieved.
— 
Dan

Re: Disable C++ exceptions (issue 571430048 by jonas.hahnf...@gmail.com)

2020-01-28 Thread jonas . hahnfeld

Reviewers: lemzwerg, Dan Eble,

Message:
On 2020/01/27 21:05:27, Dan Eble wrote:
> > 1) Drop exception handling around scm_boot_guile
> > Doing this at the end of main() is pointless.
> 
> Agreed.  (It should have been caught by reference anyway.)
> 
> > 2) Disable exception handling code
> > We don't throw in LilyPond and it's only bloating the executable:
> > before: 192,586,512B
> > after:  166,845,952B (-13.3%)
> 
> A. Do we care that much about about 30 MB?  Is someone complaining?

Yes, I care. Why waste space if we don't have to?

> B. The standard library may throw for various reasons.

As far as I know, it calls abort() instead of raising exceptions when
compiled with -fno-exceptions. Even if not, libc will catch the
exception and show the result of e.what().

> > This might also have a positive effect on the build time
> 
> I have no experience with this.

Short unscientific experiment on my otherwise idle laptop from 2016
(make -j4):
before:
real6m34,192s
user20m12,628s
sys 1m18,162s

after:
real6m12,393s
user18m48,599s
sys 1m15,815s

> > as well as performance during runtime
> 
> Not likely.  The cost is about zero until one is thrown.  I've worked
on
> high-availability, high-performance network packet processing products
where we
> used exceptions to good effect.

Agreed with the generated exception handling code on Linux, I might have
confused with the older version from Windows. But even they have a low
overhead nowadays.

> I don't have a better reason to oppose disabling exceptions than that
I've found
> them useful in other projects, so I'll say LGTM; but I think we could
offer
> posterity a better justification than 30 MB and maybe build time.
> 
> To me, the most convincing argument for disabling exceptions (which in
g++ turns
> them into abort(), IIUC; I'm not sure about clang) would be that you
don't want
> LilyPond contributors to have to write exception-safe code.  It's not
natural
> for C programmers coming into C++.  Also, I'm sure you've already got
some.

Just my opinion: I think many projects work well without exceptions,
with GCC and LLVM being two large ones that I'm used to. At the moment,
I don't see a valid use case for exceptions in LilyPond, but I haven't
seen all of the code yet. If exceptions prove helpful in the future, we
can certainly think about switching them back on.

Description:
Disable C++ exceptions

1) Drop exception handling around scm_boot_guile

Doing this at the end of main() is pointless.

2) Disable exception handling code

We don't throw in LilyPond and it's only bloating the executable:
before: 192,586,512B
after:  166,845,952B (-13.3%)

This might also have a positive effect on the build time as well as
performance during runtime, but I didn't try to measure that for now.

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

Affected files (+4, -16 lines):
  M lily/main.cc
  M stepmake/stepmake/c++-vars.make
  M stepmake/stepmake/test-rules.make


Index: lily/main.cc
diff --git a/lily/main.cc b/lily/main.cc
index 
b23c1596e7b3d1d0247e5a6664fae1bca4e48b53..a8965712bcd0a2974866392418400b88aee79f9c
 100644
--- a/lily/main.cc
+++ b/lily/main.cc
@@ -818,22 +818,7 @@ main (int argc, char **argv, char **envp)
   /*
* Start up Guile API using main_with_guile as a callback.
*/
-#if (GUILEV2)
- /* Debugging aid.
-Set it on by default for Guile V2
-while migration in progress.
- */
-  try
-{
-  scm_boot_guile (argc, argv, main_with_guile, 0);
-}
-  catch (std::exception e)
-{
-  error (_f ("exception caught: %s", e.what ()));
-};
-#else
   scm_boot_guile (argc, argv, main_with_guile, 0);
-#endif
 
   /* Only reachable if GUILE exits.  That is an error.  */
   return 1;
Index: stepmake/stepmake/c++-vars.make
diff --git a/stepmake/stepmake/c++-vars.make b/stepmake/stepmake/c++-vars.make
index 
73dd91e240170d11f7736424e7da81ba6c21453a..9a011d3fb9ef8761f200d67ed7358084b810
 100644
--- a/stepmake/stepmake/c++-vars.make
+++ b/stepmake/stepmake/c++-vars.make
@@ -14,7 +14,7 @@ DO_O_DEP = rm -f $(o-dep-out); 
DEPENDENCIES_OUTPUT="$(o-dep-out) $(outdir)/$(not
 lo-dep-out = $(outdir)/$(subst .lo,.dep,$(notdir $@))#
 DO_LO_DEP = rm -f $(lo-dep-out); DEPENDENCIES_OUTPUT="$(lo-dep-out) 
$(outdir)/$(notdir $@)"
 
-EXTRA_CXXFLAGS = -std=c++11 -W -Wall -Wconversion -Woverloaded-virtual
+EXTRA_CXXFLAGS = -std=c++11 -fno-exceptions -W -Wall -Wconversion 
-Woverloaded-virtual
 #ifeq ($(MY_PATCH_LEVEL),)
 #EXTRA_CXXFLAGS += -Werror
 #endif
Index: stepmake/stepmake/test-rules.make
diff --git a/stepmake/stepmake/test-rules.make 
b/stepmake/stepmake/test-rules.make
index 
63276b9e5c2385fd5fa8160166d61c11e0edd654..7485dc1a5a5bb4b4a702aad96b2d3dd065289e45
 100644
--- a/stepmake/stepmake/test-rules.make
+++ b/stepmake/stepmake/test-rules.make
@@ -6,6 +6,9 @@ endef
 
 $(foreach a, $(MODULE_LIBS), $(eval $(call MODULE_LIB_template,$(a
 
+# yaffut.hh catches all exceptions, so re-enable -fexceptions for

Re: automated formatting

2020-01-28 Thread Carl Sorensen

On 1/28/20, 1:50 AM, "lilypond-devel on behalf of Han-Wen Nienhuys" 
 wrote:

On Tue, Jan 28, 2020 at 12:18 AM David Kastrup  wrote:
>
> David Kastrup  writes:
>
> > Han-Wen Nienhuys  writes:
> >
> >> On Mon, Jan 27, 2020 at 11:49 AM David Kastrup  wrote:
> >>> > I want to propose to move to automated formatting for our C++ code.
> >>> >
> >>> > I put up a .clang-format code that mostly mimicks our style at
> >>> >
> >>> >   https://codereview.appspot.com/561340043
> >>> >
> >>> > I have a lot of good experience with automating code formatting.. It
> >>> > removes drudgery for code authors, obviates discussions over style 
in
> >>> > code review, and generally elevates the level of discourse in our
> >>> > reviews.
> >>> >
> >>> > What do you all think?
> >>>
> >>> scripts/auxiliar/fixcc.py and astyle 2.04 is what we standardized on.
> >>
> >> Yes, I am proposing that we change and standardize on clang-format.
> >>
> >>> > The current config modifies about 11k lines, mostly because of
> >>> > different line breaks (necessary to keep the 80 column limit.)
> >>>
> >>> Any particular reason to change the automated style to a different 
one?
> >>
> >> Several reasons
> >>
> >> * clang-format is a *complete* formatter. It reformats files to a
> >> canonical formatting regardless of how badly you mangle the input.
> >

> >
> >> * clang-format is based on clang itself, and has an understanding of
> >> the source code being formatted. This makes it better than fixcc which
> >> uses regular expressions (sic) to make sense of C++.
>

>
> I understand that it was chiefly my neglect of our existing reformatting
> regime that caused you to look for a solution for a problem
> unnecessarily and invest time into it.  I apologize for that, and am
> willing to bear the cost in future cherry-picks into the 2.20 stable
> branch for reformatting both 2.20.0 and 2.21.0 before release.
>
> But the additional cost for changing to a completely different
> reformatting system just does not seem warranted as long as there are no
> obvious deficiencies to be seen with what we have now.

I think you are missing the larger point I have been trying to make.

In Salzburg, people asked "what scares contributors away", and "what
can we do to suck Han-Wen/Mike back into the project"?

The project cares about code formatting, as evidenced by fixcc.py.
However, having to deal with code formatting, both while writing code,
and during review, is an absolute turn-off for me. If you want to suck
me (and others) in, you have to make the code formatting process as
transparent and automatic as possible.

I admit that when I saw a request for clang as a requirement for development I 
was somewhat dismayed.  Like David, I felt like we had a workable tool, and we 
could stick with it.

HOWEVER, if moving to clang-format is a price we have to pay to have Han-Wen 
contributing his expertise to the project, I would willingly pay it. 

UNLESS moving to clang-format would drive David K away from contributing his 
expertise to the project.

I really hope we can find a solution that will satisfy both David and Han-Wen, 
because I think that both of them are instrumental to the health of LilyPond.  
And I love having LilyPond available!

Can you provide me with a presubmit hook that applies fixcc? Can you
guarantee that, if fixcc has run on the code, there will be no further
remarks on code formatting during review?

I think that it's a really good idea to have presubmit hooks.  I believe, but 
can't guarantee, that if all code were automatically reformatted on submission 
(by either fixcc or clang-format) there would be virtually no comments on 
formatting.  And you could completely ignore any that were made.

The point with completely automatic formatting is that you can simply
add a rule to "make check" (or some sort of "check" if we were to use
github or gitlab) that prevents the code ever to stray from the
defined standard.

I agree that completely automatic formatting is a worthy goal.  I think we 
should have it.   If we can only have it with clang-format, then we should 
probably use clang-format.  If we can have it with both clang-format and fixcc, 
then the decision between the two tools needs to be based on criteria other 
than completely automatic formatting.

I know that Dan also has issues with the current code formatting process.

If you reject clang-format (on grounds that I don't agree with, BTW),
then can you come up with another solution that lets me (and other
contributors) stop worrying about formatting in some other way? Note
that I can't even run fixcc (see below).

BTW, If you want to peruse clang-format output, you can go to

Re: Looking for help in configuring LilyDev in VirtualBox

2020-01-28 Thread Peter Toye

Tuesday, January 28, 2020, 1:38:45 PM, Federico Bruni wrote:

> Il giorno gio 23 gen 2020 alle 22:11, Federico Bruni
>  ha scritto:
>> I will let you know when v2 is released.

> Now released:
> 

Grazie molto


Best regards,

Peter

Re: Data structure for (package) options

2020-01-28 Thread Urs Liska




Am 28. Januar 2020 14:48:54 MEZ schrieb David Kastrup :
>Han-Wen Nienhuys  writes:
>
>> On Mon, Jan 27, 2020 at 11:39 PM Urs Liska 
>wrote:
>>>
>>> I didn't have time to really think about much (about LilyPond) the
>past
>>> week, just enjoyed seeing so much constructive discussion.
>>> [..]
>>
>> I haven't read your messages in detail, but I'd like to throw out one
>> thought to consider: we use GUILE modules as a mechanism for
>> identifier namespacing (\paper, \header all create modules). I think
>> it might be useful if we could provide a LilyPond native mechanism
>for
>> packaging that declares a namespace implicitly, eg.
>>
>>   \module "edition" {
>>   internal = ...
>>   addTweak  = ...
>>   }
>>
>>   \import edition.addTweak
>>
>> that would let you define constructs in a package that doesn't
>pollute
>> the global namespace, and let .ly packages control explicitly what
>> symbols they want to use.
>>
>> I think we should aim to avoid textual inclusion as a mechanism that
>> powers packaging.
>
>At the implementation side, "textual inclusion" will be what has to
>power systems where one can "plug in" packages and have them work by
>being present.  Even Guile's use-modules mechanism works at that level.
>But of course that doesn't mean that we want \include as a user-level
>access method in the long haul.
>
>It's likely that a typical more complex package may consist of both .ly
>and/or .scm components and that is an implementation detail that a
>package user should not need to bother about, and that hopefully should
>not significantly complicate things for people wanting to provide
>packages with either or both .ly and/or .scm components.

From my experience with OLL it would sesm reasonable to have all packages use 
.ly files as entry point. From there they can use Scheme modules if they need 
to. OLL does some work to add its root to the guile path, so there's the way to 
use modules relstive to the package.

Urs


-- 
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.

Re: Data structure for (package) options

Han-Wen Nienhuys  writes:

> On Mon, Jan 27, 2020 at 11:39 PM Urs Liska  wrote:
>>
>> I didn't have time to really think about much (about LilyPond) the past
>> week, just enjoyed seeing so much constructive discussion.
>> [..]
>
> I haven't read your messages in detail, but I'd like to throw out one
> thought to consider: we use GUILE modules as a mechanism for
> identifier namespacing (\paper, \header all create modules). I think
> it might be useful if we could provide a LilyPond native mechanism for
> packaging that declares a namespace implicitly, eg.
>
>   \module "edition" {
>   internal = ...
>   addTweak  = ...
>   }
>
>   \import edition.addTweak
>
> that would let you define constructs in a package that doesn't pollute
> the global namespace, and let .ly packages control explicitly what
> symbols they want to use.
>
> I think we should aim to avoid textual inclusion as a mechanism that
> powers packaging.

At the implementation side, "textual inclusion" will be what has to
power systems where one can "plug in" packages and have them work by
being present.  Even Guile's use-modules mechanism works at that level.
But of course that doesn't mean that we want \include as a user-level
access method in the long haul.

It's likely that a typical more complex package may consist of both .ly
and/or .scm components and that is an implementation detail that a
package user should not need to bother about, and that hopefully should
not significantly complicate things for people wanting to provide
packages with either or both .ly and/or .scm components.

-- 
David Kastrup

Re: Looking for help in configuring LilyDev in VirtualBox

2020-01-28 Thread Federico Bruni

Il giorno gio 23 gen 2020 alle 22:11, Federico Bruni 
 ha scritto:

I will let you know when v2 is released.


Now released:

Re: Data structure for (package) options