Re: Data structure for (package) options

[Urs Liska]

Am Dienstag, den 28.01.2020, 00:34 +0100 schrieb David Kastrup:
> Urs Liska  writes:
> 
> > I didn't have time to really think about much (about LilyPond) the
> > past
> > week, just enjoyed seeing so much constructive discussion.
> > 
> > I think the first thing I'd like to go for with the
> > package/extension
> > mechanism is storing and handling (package) options.
> > 
> > There are three use cases which are differently closely related to
> > the
> > actual package mechanism but should be dealt with together:
> > 
> >  * package options
> > * specified, typed and preset with default values by a package
> > * handled by getter and setter functions
> >  * custom options
> > * behave like package options
> > * not associated with a package but explicitly called for in
> > user
> >   code
> >  * custom data
> > * use the option syntax/infrastructure to store arbitrary data
> > * for example a music tree as described in Jan-Peter's
> > templating
> > 
> > I think package options should be handled in one association list,
> > hidden in a Scheme module and only accessed by specific accessor
> > code.
> > The list elements should have the package name as car and an option
> > *object* as cdr:
> > 
> >   (define package-options
> >'((edition-engraver . ee-options)
> >  (scholarly . scholarly-options)
> >  ...
> > ))
> > 
> > I expect the number of packages loaded in a document ranging from
> > "a
> > few" to "a few dozens", so probably a simple alist should be the
> > right
> > data structure?
> 
> Before we devolve into an efficiency discussion here, let me sketch
> one
> of my "should make sense" projects": our access of alists mostly is
> through our own accessor function ly:assoc-get .  If the elements on
> an
> alist could not just be a key-value pair but a hashtable (for looking
> up
> key-value, of course) or a vector (I think I'd prefer it being 1-
> based
> in spite of making the Scheme indexing less obvious), our lookup
> routines should be able to deal with that.  Some alists could be
> compressed on use, making things like drumtables and note alists
> efficient behind the scenes.
> 
> Such a replacement would work transparently for things like
> 
> package-options.edition-engraver = #'ee-options
> 
> or similar.  So basically there is long-term potential for efficiency
> to
> mostly become a non-issue.

I'm sorry, but I don't fully understand what you are trying to tell me
here. Are you saying that you already had some internal changes in mind
and that storing the package options in an alist is something I
shouldn't really worry about?

BTW: I mistook my own example (shouldn't have started writing late in
the evening ...). What I meant is storing the options in objects, not
assigning symbols:

  (define package-options
   `((edition-engraver . ,ee-options)
 (scholarly . ,scholarly-options)
 ...
))

with ee-options and scholarly-options having been bound to tree objects
before.

> 
> > It seems reasonable to not store a package's options as a nested
> > alist,
> > though. I'd rather consider using a tree implementation in a class,
> > which would for example make it cleaner to encapsulate the
> > behaviour
> > and e.g. implement type checking in that class rather than in the
> > accessor functions, or enable alternative tree implementations if
> > that
> > should become interesting for custom data storage.
> > We have a tree implementation in oll-core at 
> > https://github.com/openlilylib/oll-core/blob/master/scheme/oll-core/tree.scm
> > Would that be something to use here?
> 
> Whatever we do, it should be an implementation detail the user _and_
> the
> package programmers do not really need to know about.  There should
> be
> accessors hiding the organisation.
> 

Yes, that's what I intended. It should be a tree implementation with a
given characteristic that basically noone should bother about. There
should be the *possibility* to drop-in a different tree implementation
(with the same interface) if for whatever reason a package might want
to do so (I could imagine that for some algorithmic use of the data it
might be reasonable to consider some sort of self-organizing tree to
optimize access times - but definitely not for the bread-and-butter use
case of package options).

Urs

Issue 5705: int->long in Stem::get_beaming and set_beaming (issue 561350044 by nine.fierce.ball...@gmail.com)

[lemzwerg--- via Discussions on LilyPond development]

LGTM

https://codereview.appspot.com/561350044/

Re: Data structure for (package) options

[David Kastrup]

Urs Liska  writes:

> I didn't have time to really think about much (about LilyPond) the past
> week, just enjoyed seeing so much constructive discussion.
>
> I think the first thing I'd like to go for with the package/extension
> mechanism is storing and handling (package) options.
>
> There are three use cases which are differently closely related to the
> actual package mechanism but should be dealt with together:
>
>  * package options
> * specified, typed and preset with default values by a package
> * handled by getter and setter functions
>  * custom options
> * behave like package options
> * not associated with a package but explicitly called for in user
>   code
>  * custom data
> * use the option syntax/infrastructure to store arbitrary data
> * for example a music tree as described in Jan-Peter's templating
>
> I think package options should be handled in one association list,
> hidden in a Scheme module and only accessed by specific accessor code.
> The list elements should have the package name as car and an option
> *object* as cdr:
>
>   (define package-options
>'((edition-engraver . ee-options)
>  (scholarly . scholarly-options)
>  ...
> ))
>
> I expect the number of packages loaded in a document ranging from "a
> few" to "a few dozens", so probably a simple alist should be the right
> data structure?

Before we devolve into an efficiency discussion here, let me sketch one
of my "should make sense" projects": our access of alists mostly is
through our own accessor function ly:assoc-get .  If the elements on an
alist could not just be a key-value pair but a hashtable (for looking up
key-value, of course) or a vector (I think I'd prefer it being 1-based
in spite of making the Scheme indexing less obvious), our lookup
routines should be able to deal with that.  Some alists could be
compressed on use, making things like drumtables and note alists
efficient behind the scenes.

Such a replacement would work transparently for things like

package-options.edition-engraver = #'ee-options

or similar.  So basically there is long-term potential for efficiency to
mostly become a non-issue.

> It seems reasonable to not store a package's options as a nested alist,
> though. I'd rather consider using a tree implementation in a class,
> which would for example make it cleaner to encapsulate the behaviour
> and e.g. implement type checking in that class rather than in the
> accessor functions, or enable alternative tree implementations if that
> should become interesting for custom data storage.
> We have a tree implementation in oll-core at 
> https://github.com/openlilylib/oll-core/blob/master/scheme/oll-core/tree.scm
> Would that be something to use here?

Whatever we do, it should be an implementation detail the user _and_ the
package programmers do not really need to know about.  There should be
accessors hiding the organisation.

-- 
David Kastrup

Re: automated formatting

[David Kastrup]

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.
>
> Do we want to mangle the input badly?
>
>> For example, if you introduce a comment line of 200 chars. In
>> clang-format, this will be reformatted to the specified column limit.
>> Astyle/fixcc doesn't know what to do with it.
>
> Comments often contain ASCII-art and formatted tables.  Formatting those
> is not helpful.

Here are examples:

  /*
   * this case (distant half collide),
   *
   *|
   *  x |
   * | x
   * |
   *
   * the noteheads may be closer than this case (close half collide)
   *
   *|
   *|
   *   x
   *  x
   * |
   * |
   *
   */


  /* TODO:

  For some cases we should kern some more: when the
  distance between the next or prev note is too large, we'd
  get large white gaps, eg.

  |
  X|
  |X  <- kern this.
  |
  X

  */


>
>> * 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++.

For better or worse, we still do a lot with preprocessor macros that are
applied in the manner of declarations and statements.  They are
formatted in a superficial manner, reflecting the kind of construct they
are mimicking (declarations, function calls) for the sake of human
readers.  Reflecting the underlying C++ realities instead would be a
distraction.  I don't want to diss Clang here: it is unlikely that it
would actually dig down to the C++ level for analysing these constructs
instead of staying at the preprocessor level.  But what I am saying is
that we usually want to make code readable to _humans_ and that means
patterning it in a manner where deep semantic analysis should not be a
requirement.

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.

-- 
David Kastrup

Re: automated formatting

[David Kastrup]

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.

Do we want to mangle the input badly?

> For example, if you introduce a comment line of 200 chars. In
> clang-format, this will be reformatted to the specified column limit.
> Astyle/fixcc doesn't know what to do with it.

Comments often contain ASCII-art and formatted tables.  Formatting those
is not helpful.

> * 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++. This is
> especially important as we move towards newer dialects of  C++ with
> newer constructs (eg. lambda).

C++11 support is already in the release notes for Astyle 2.03 in 2013.

> * Because it is so robust, it is safe to run automatically on save,
> and there exists VIM and Emacs support to do so

I don't understand what you call "safe" and "robust" here.  Do you
suggest that using Astyle would change the meaning of C++ code?

> * fixcc is 500 lines of python that we could just stop maintaining.

Fixcc does not much more than drive Astyle, and Astyle is being
maintained by someone else.  Here is its dependency list for the sake of
people using lily-dev or similar:

Depends: libc6 (>= 2.14), libstdc++6 (>= 5.2)

Here is the dependency list of clang-format:

Depends: libc6 (>= 2.14), libclang-cpp9, libgcc1 (>= 1:3.0), libllvm9 (= 
1:9-2), libstdc++6 (>= 5.2), python3

So one dependency is python3, another is libclang-cpp9, and then there
are various other compiler parts (both GCC and LLVM).

I have put up an issue showing the current job fixcc/astyle would do on
our code base.  Do you see significant problems showing up there?

> * clang-format is fast. I can format all of the LilyPond C++ code in 4
> seconds. That makes it fast enough to stick into a pre-commit hook
> (check changed files for formatting).

time scripts/auxiliar/fixcc.py --sloppy $(git ls-files '*.cc' '*.hh')

real0m12.651s
user0m10.593s
sys 0m2.057s

and I'd be willing to bet that your laptop runs faster than mine with
its Intel(R) Core(TM) i7-2630QM CPU @ 2.00GHz .

> * clang-format is configurable. I whipped a config that I think
> matches what we have, but I might have missed some options. Ideally,
> the changes would be small.

The changes for AStyle are small because we already use AStyle.

> * clang-format is well-established. Both the linux kernel and Git have
> a .clang-format files in their trees.

AStyle is well-established since we have been using it for years without
a problem.  If we get problems, that may warrant reevaluation.

Can you point out problems in the sample I posted as

Current branch: issue5703
Tracker issue: 5703 (https://sourceforge.net/p/testlilyissues/issues/5703/)
Rietveld issue: 549480043 (https://codereview.appspot.com/549480043)
Issue description:
  Run scripts/auxiliar/fixcc.py  This is just intended to showcase the
  current fixes fixcc would cause.

?

> Finally, commenting on Werner: there are two main positives for
> automated formatting:

I am not going to argue here since that is an old discussion with a
resolution way in the past under Graham's tenure, and we opted for it.
I forgot about the proper upkeep, and your reminder is very much
appreciated.  But we do have a working solution right now, even if it is
my fault not to have minded it properly, and changing to one with vastly
more dependencies and having to redefine formatting from scratch is
something that I cannot, in the current situation, see as a winning
proposition.

In particular, pinning the operation to a particular version of Clang
and making it automatic in an editor would make it a considerable
headache for people to _not_ accidentally stomp back and forth over the
formatting of files when their Clang version does not match that of
other people.

> 2. When code has standard formatting, it is much easier to build
> automation for code changes, or do 

Data structure for (package) options

[Urs Liska]

I didn't have time to really think about much (about LilyPond) the past
week, just enjoyed seeing so much constructive discussion.

I think the first thing I'd like to go for with the package/extension
mechanism is storing and handling (package) options.

There are three use cases which are differently closely related to the
actual package mechanism but should be dealt with together:

 * package options
* specified, typed and preset with default values by a package
* handled by getter and setter functions
 * custom options
* behave like package options
* not associated with a package but explicitly called for in user
  code
 * custom data
* use the option syntax/infrastructure to store arbitrary data
* for example a music tree as described in Jan-Peter's templating

I think package options should be handled in one association list,
hidden in a Scheme module and only accessed by specific accessor code.
The list elements should have the package name as car and an option
*object* as cdr:

  (define package-options
   '((edition-engraver . ee-options)
 (scholarly . scholarly-options)
 ...
))

I expect the number of packages loaded in a document ranging from "a
few" to "a few dozens", so probably a simple alist should be the right
data structure?

Package and custom config options are organized as a tree. From my
experience so far the nodes in that tree have 1-10 children each, while
I can imagine this to be 20 to 30 or even 50 in rare cases, but surely
not more.

It seems reasonable to not store a package's options as a nested alist,
though. I'd rather consider using a tree implementation in a class,
which would for example make it cleaner to encapsulate the behaviour
and e.g. implement type checking in that class rather than in the
accessor functions, or enable alternative tree implementations if that
should become interesting for custom data storage.
We have a tree implementation in oll-core at 
https://github.com/openlilylib/oll-core/blob/master/scheme/oll-core/tree.scm
Would that be something to use here?

My idea is to put that in an .scm file, say, scm/options.scm, and the
accessor code in a file ly/options.ly.

Additionally there's a file ly/packages.ly that will define a
\usepackage command. \usepackage will check if there's already an
element in the options alist for the given package name, and if not it
will create one with a new tree object. The package file is then free
to register options which will implicitly be stored in the tree object.

Does that look like a viable approach?

Urs

Re: automated formatting

[Han-Wen Nienhuys]

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.

For example, if you introduce a comment line of 200 chars. In
clang-format, this will be reformatted to the specified column limit.
Astyle/fixcc doesn't know what to do with it.

* 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++. This is
especially important as we move towards newer dialects of  C++ with
newer constructs (eg. lambda).

* Because it is so robust, it is safe to run automatically on save,
and there exists VIM and Emacs support to do so

* fixcc is 500 lines of python that we could just stop maintaining.

* clang-format is fast. I can format all of the LilyPond C++ code in 4
seconds. That makes it fast enough to stick into a pre-commit hook
(check changed files for formatting).

* clang-format is configurable. I whipped a config that I think
matches what we have, but I might have missed some options. Ideally,
the changes would be small.

* clang-format is well-established. Both the linux kernel and Git have
a .clang-format files in their trees.

Finally, commenting on Werner: there are two main positives for
automated formatting:

1. you can stop caring about formatting complely, ie.

> The 80 column limit is a special beast.  Sometimes it *does* make
> sense to not follow this restriction, especially if the excess is just
> a few characters...

when the formatting is completely automatic, there is "follow" or "not
follow". You simply press "Save" and the code is formatted as it is
supposed to be. Maybe that means the result is slightly suboptimal in
specific situations, but overall, not having to think about formatting
is very liberating.

2. When code has standard formatting, it is much easier to build
automation for code changes, or do global search/replaces over the
code base. (This is probably not a big factor for LilyPond, but it's
what makes large scale automated refactoring feasible at places like
Google.)

> Clang is a pretty big dependency for developers.

You'd think that, but it's not that bad:

$ rpm -qi clang| grep Size
Size: 1695283

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

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

[nine . fierce . ballads]

> 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?
B. The standard library may throw for various reasons.

> This might also have a positive effect on the build time

I have no experience with this.

> 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.

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.

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

Re: GUILE 2.2 progress

[Thomas Morley]

Am So., 26. Jan. 2020 um 14:41 Uhr schrieb Han-Wen Nienhuys :
>
> See https://github.com/hanwen/lilypond/pull/1
>
> This has a stack of dependent commits (see below). I don't know how to
> deal with this on Rietveld.
>
> commit 3c8081d25b53529efe687ffe9e92ef48f5d11ea2
> Author: Han-Wen Nienhuys 
> Date:   Sun Jan 26 12:19:56 2020 +0100
>
> Parse inline scheme using Overlay_string_port
>
> Overlay_string_port makes a port view of the existing data. This port
> is used for parsing the embedded Scheme form, and is then discarded.
>
> The port view uses UTF-8 encoding, so UTF-8 encoded string constants
> within the Scheme constants are interpreted as Unicode correctly.
>
> This obviates the string port that Source_file carries along.
>
> commit ab765369321a0fc9256b78b74ef98c23ba7498f3
> Author: Han-Wen Nienhuys 
> Date:   Sun Jan 26 12:19:49 2020 +0100
>
> Overlay_string_port
>
> commit 9188f7f65992fc8768402cfec3ec55a8c2adabd2
> Author: Han-Wen Nienhuys 
> Date:   Sun Jan 26 11:16:43 2020 +0100
>
> Clean up embedded scheme parsing/evaluation.
>
> Renames and reorders functions to clarify the mechanism. No functional
> changes.
>
> --
> Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen
>

I' m currently compiling it[*] with guile-2.2.6.

And will make some experiments, if all goes well.
It will take same time before I get results, my regular job eats too
much of my time.

Is https://sourceforge.net/p/testlilyissues/issues/5699/ meant to
collect bugs and other observations?

I'm going to loose oversight of the current state, do we have a TODO-list?

Best,
  Harm

[*] Just to make sure we talk about the same, my compilation is done with:

hermann@kasten ~/lilypond-git-hanwen/build (test-27-1-2020)$ git log --oneline
58d4250dda (HEAD -> test-27-1-2020) Introduce ly_scm2utf8_string, and
use it for printing text
511a657ded Parse inline scheme using Overlay_string_port
abaa6ff4f4 Introduce a throw-away string port
9188f7f659 Clean up embedded scheme parsing/evaluation.
f7d1f92237 Use namespace for catching std::exception
a6dcf561b1 GUILE v2: set postscript output port to Latin1
85c39ffa16 Fix ly:protects compile for GUILE 2.2
23816a096c In verbose mode, dump time spent on parsing lily.scm and friends.
23d5fc99a7 Instrument and improve GC time
c5ffa540fd Issue 5673: Remove implicit evaluation of the ".twy" file
d727336621 Issue 5671: lily/page-breaking: pass vector by reference
a148afbd01 Issue 5669: Document why System::rank_type is int16
54bb3f44ff Issue 5668: Documentation: fix typo in German spacing.itely file
f0ab8ef8b3 Issue 5667: Documentation: fix typos in tool naming
0f046f4f33 Issue 5666: ly_scm_write_string: call scm_write directly
c58c84ef81 Issue 5675: Document C++ structs for slur scoring
9f8b7f0d6d Issue 5676: Remove obsolete lines from lily-guile-macros.hh
8ceb4b856a Doc: Added documentation for fill-line line-width
00929c7b4b Issue 4550/3: Remove remaining "using namespace std;" with a script
6f4128e1f3 Issue 4550/2: Avoid "using namespace std;" in included files
9dc0905b76 Issue 4550/1: Remove flower/include/parray.hh
5534def446 (origin/scm-stl) Issue 5665/5: Update documentation about
tracing options
d2f8974223 Issue 5665/4: Delete script build-profile.sh
c167b0140a Issue 5665/3: Delete scripts to measure coverage
1c91eb18cc Issue 5665/2: Drop -dtrace-memory-frequency
626b35da58 Issue 5665/1: Drop -dtrace-scheme-coverage
13877f0d5e Issue 5663/3: lilylib: Remove encoded_write
5ee6b58300 Issue 5663/2: musicxml2ly: Correct usage of codecs module
e0c78a4c71 Issue 5663/1: Use codecs.open() to decode as utf-8
0471360de0 Issue 5654/4: Remove passing of package variables
90e0a31201 Issue 5654/3: Call AC_INIT correctly with all parameters
2db727328d Issue 5654/2: Drop unused MICRO_VERSION
194a0001cd Issue 5654/1: Cleanup directory handling in STEPMAKE_INIT
bea1f4bcd1 Run `grand-replace' script to update copyright year.
5040c55095 Issue 5661: Fix some conversion warnings in Source_file.
9cf6b20aef Issue 5659/8: Remove Interval_t::T_to_string ()
a66ebb85d9 Issue 5659/7: Finally remove deleted ::to_string () overloads
834319e8c0 Issue 5659/6: Delete ::to_string (i) for various integer types
37bd476389 Issue 5659/5: Delete ::to_string (bool)
a02d3f0af7 Issue 5659/4: Delete ::to_string (char, ssize_t)
8ea3c33201 Issue 5659/3: Delete ::to_string (const string&)
8c3c5a4cad Issue 5659/2: Remove String_convert::char_string
446dbaff54 Issue 5659/1: Remove some unused string-related functions
67b330d32f Issue 5662: Remove shebang from langdefs.py
2079393c36 Issue 5652: Fix a few complaints in python/convertrules.py
f2fb762a09 Issue 5309/2: find_global_context ()
b0eb50c2b1 Issue 5309/1: find_score_context ()
20c9983553 Issue 5658/3: Cleanup: !isinf && !isnan == isfinite
bb711884c6 Issue 5658/2: Qualify isinf, isnan, isfinite with std::
0376c35dbd Issue 5658/1: Include  consistently, not 
a2bb24fed4 Issue 5655 general-column does not move empty stencils
[...]

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

[lemzwerg--- via Discussions on LilyPond development]

LGTM


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

Re: UTF-8 and default locale

[David Kastrup]

Han-Wen Nienhuys  writes:

> Hey David,
>
> before we go into formal review, could you have a look at
>
> https://github.com/hanwen/lilypond/commit/3402c61aa0d6f36b8dce984947d61ba01b0a6350
>
> The commits fix Unicode lyrics,  but maybe we want to change the
> setlocale() calls in main instead.

The patch annoys me because of the extra string copy, but then that's
the same as previously and no obvious way to avoid it.  It's more a
C/C++ thing than a Guile one.

I am not sure about the setlocale calls in main: in principle, an
application should talk to the console in the local locale independent
from what it does on file-I/O.  In practice, I think it would be a long
haul to get LilyPond to work in non-UTF8 console locales.

-- 
David Kastrup

Re: Issue 5698: int->vsize in various alignment and page-layout methods (issue 563420043 by nine.fierce.ball...@gmail.com)

[Dan Eble]

On Jan 27, 2020, at 10:13, David Kastrup  wrote:
> 
> None relevant to GCC and Clang as far as I can see.  Can you clarify for
> which platforms you expect a practical concern?

No.  That would require a level of effort that is a reason I never mentioned 
these things before.
— 
Dan

Re: Issue 5698: int->vsize in various alignment and page-layout methods (issue 563420043 by nine.fierce.ball...@gmail.com)

[David Kastrup]

Dan Eble  writes:

> On Jan 27, 2020, at 09:33, David Kastrup  wrote:
>> 
>>> I would like to replace the following with standard types (uint8_t
>>> etc.).  The standard types are portable, but these are not.
> ...
>>>  * flower-proto.hh:typedef unsigned U32;
>>>  * flower-proto.hh:typedef int I32;
> ...
>> I don't really mind here, but "portable" practically means just portable
>> to GCC and Clang which closely follows GCC, so it's not a problem in
>
> I was thinking of differences in data model, e.g. "unsigned" and "int"
> might have as few as 16 bits.  See some examples at
> https://en.cppreference.com/w/cpp/language/types .

None relevant to GCC and Clang as far as I can see.  Can you clarify for
which platforms you expect a practical concern?

-- 
David Kastrup

Re: Issue 5698: int->vsize in various alignment and page-layout methods (issue 563420043 by nine.fierce.ball...@gmail.com)

[Dan Eble]

On Jan 27, 2020, at 09:33, David Kastrup  wrote:
> 
>> I would like to replace the following with standard types (uint8_t
>> etc.).  The standard types are portable, but these are not.
...
>>  * flower-proto.hh:typedef unsigned U32;
>>  * flower-proto.hh:typedef int I32;
...
> I don't really mind here, but "portable" practically means just portable
> to GCC and Clang which closely follows GCC, so it's not a problem in

I was thinking of differences in data model, e.g. "unsigned" and "int" might 
have as few as 16 bits.  See some examples at 
https://en.cppreference.com/w/cpp/language/types .
— 
Dan

Re: Issue 5698: int->vsize in various alignment and page-layout methods (issue 563420043 by nine.fierce.ball...@gmail.com)

[David Kastrup]

Dan Eble  writes:

> On Jan 27, 2020, at 03:26, hanw...@gmail.com wrote:
>> 
>> LGTM
>> 
>> Not for this change, but could we do a global 
>> 
>>  vsize -> size_t
>> 
>> search & replace.  Do we have a reason to keep our own typedef for this?
>
> I fully support this.  I wasn't going to bring it up yet because I
> worried that it would lead to a lot of discussion and I have bigger
> fish to fry, but I would like to eliminate the following from flower
> because they reduce clarity.
>
>   * real.hh:typedef double Real;
>   * std-string.hh:typedef size_t ssize;
>   * std-vector.hh:typedef size_t vsize;

They fix certain types that are used consistently.  If you say they
"reduce clarity", then every typedef reduces clarity.

ssize and vsize may depend on our internal implementations of
std-string.hh and std-vector.hh.  With the current code base, as far as
I know our own types are gone, so size_t seems fine for replacement.

I am decidedly less sure about Real.  Different sizes than just double
might make sense where it is used, say, as the base type of Offset.  I
am not overly sure that we use it consistently, though.

> I would like to replace the following with standard types (uint8_t
> etc.).  The standard types are portable, but these are not.
>
>   * flower-proto.hh:typedef unsigned char Byte;
>   * flower-proto.hh:typedef long long I64;
>   * flower-proto.hh:typedef unsigned char U8;
>   * flower-proto.hh:typedef short I16;
>   * flower-proto.hh:typedef unsigned short U16;
>   * flower-proto.hh:typedef unsigned U32;
>   * flower-proto.hh:typedef int I32;
>   * flower-proto.hh:typedef unsigned long long U64;

I don't really mind here, but "portable" practically means just portable
to GCC and Clang which closely follows GCC, so it's not a problem in
practice but at most in readability, and the names are clear enough.

At any rate, like with running fixcc.py I think the best point of time
would be when the stable branch has seen a formal end of cherry-picking
in order not to complicate it.  The style changes can happen in parallel
on both release branches.  With regard to the somewhat more invasive
type frobbing, I think I'd wait for it until 2.21.0 has already been
out.

-- 
David Kastrup

Re: automated formatting

[Dan Eble]

On Jan 27, 2020, at 07:43, David Kastrup  wrote:
> 
> Frankly, I just forgot about either.  I think it would be a good idea to
> take up this practice again.  Regarding Clang: unless Astyle falls apart
> using C++11 constructs that are going to occur more frequently in
> future, I think there is little reason to depart from its use instead of
> reverting to Clang.

I have learned not to fuss about the particulars of required style as long as I 
have the following:

1. a simple way to exclude sections of code manually

2. a style check integrated into "make check"

3. a makefile target that automatically restyles all source so that "make 
check" will pass.  Only the files that need to be modified are touched.  This 
runs only when requested, never as a prerequisite of any other target.
— 
Dan

Re: PATCHES - Countdown for January 27th

[Jonas Hahnfeld]

Am Montag, den 27.01.2020, 12:38 +0100 schrieb James:
>  Hello,
> 
>  Here is the current patch countdown list. The next countdown will be on
>  January 29th
> 
>  A quick synopsis of all patches currently in the review process can be
>  found here:
> 
> http://philholmes.net/lilypond/allura/
>  [1]
> 
>  
>  Push:
> 
> No patches to Push at this time.
> 
> COUNTDOWN:
> 
> [...]
> 
> REVIEW:
> 
> [...]
> 
> 5646 Switch to Python 3.x - Jonas Hahnfeld
>  
> https://sourceforge.net/p/testlilyissues/issues/5646
>  [27]
> http://codereview.appspot.com/545370043

Looking at the SF issue right now, it has already moved to "countdown"?

In case you want to have a look at the squashed commit, it's in branch
dev/hahnjo/python3-push:
---
commit e99751b16e67da239c910eb6d61aba350a96be66
Author: Jonas Hahnfeld 
Date:   Mon Jan 27 14:40:04 2020 +0100

Issue 5646: Switch to Python 3.x

Find and require at least Python 3.5 which will allow us to address
some deprecation warnings, most notably about the 'imp' module.
This version was released in September 2015 and should be available
in all major distributions, including Ubuntu LTS 16.04 and 18.04 as
well as CentOS/RHEL 7.x and 8.x.

Changes folded into this commit:
 * Run "2to3 --write --nobackups ."
   The automated conversion of the remaining issues.

 * Manual changes for compatibility with Python 3
   Mostly related to encoding.

 * output-distance.py: Replace cgi.escape by html.escape
   cgi.escape() has been deprecated since Python 3.2 and was
   completely removed in the latest Python 3.8.
---

Cheers,
Jonas


signature.asc
Description: This is a digitally signed message part

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

[nine . fierce . ballads]

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);
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.

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

Re: Issue 5698: int->vsize in various alignment and page-layout methods (issue 563420043 by nine.fierce.ball...@gmail.com)

[nine . fierce . ballads]

On 2020/01/27 12:49:32, hahnjo wrote:
> Fully +1 - one step closer to eventually ditch flower completely. I
don't see a
> reason for keeping a separate library which nobody else uses...

There is some merit to flower in the area of organization and
testability.  It's nice to be able to test things like Interval_t
thoroughly (which I'm not claiming is currently the case) without
relying on the rest of LilyPond.

https://codereview.appspot.com/563420043/

Re: Run 2to3 --write --nobackups . (issue 573340043 by jonas.hahnf...@gmail.com)

[jonas . hahnfeld]

For future lookup, here's the last few lines of 2to3:
RefactoringTool: Warnings/messages while refactoring:
RefactoringTool: ### In file ./python/book_snippets.py ###
RefactoringTool: Line 716: You should use a for loop here
RefactoringTool: Line 707: You should use a for loop here
RefactoringTool: Line 738: You should use a for loop here
RefactoringTool: Line 739: You should use a for loop here
RefactoringTool: ### In file ./scripts/auxiliar/fixcc.py ###
RefactoringTool: Line 244: You should use a for loop here
RefactoringTool: ### In file ./scripts/auxiliar/makelsr.py ###
RefactoringTool: Line 304: You should use a for loop here
RefactoringTool: Line 307: You should use a for loop here
RefactoringTool: ### In file ./scripts/build/mass-link.py ###
RefactoringTool: Line 91: You should use a for loop here

Not something that needs urgent fixing, but maybe a good cleanup.

https://codereview.appspot.com/573340043/

Re: Issue 5698: int->vsize in various alignment and page-layout methods (issue 563420043 by nine.fierce.ball...@gmail.com)

[jonas . hahnfeld]

On 2020/01/27 12:19:53, dan_faithful.be wrote:
> On Jan 27, 2020, at 03:26, mailto:hanw...@gmail.com wrote:
> > 
> > LGTM
> > 
> > Not for this change, but could we do a global 
> > 
> >  vsize -> size_t
> > 
> > search & replace.  Do we have a reason to keep our own typedef for
this?
> 
> I fully support this.  I wasn't going to bring it up yet because I
worried that
> it would lead to a lot of discussion and I have bigger fish to fry,
but I would
> like to eliminate the following from flower because they reduce
clarity.
> 
>   * real.hh:typedef double Real;
>   * std-string.hh:typedef size_t ssize;
>   * std-vector.hh:typedef size_t vsize;
> 
> I would like to replace the following with standard types (uint8_t
etc.).  The
> standard types are portable, but these are not.
> 
>   * flower-proto.hh:typedef unsigned char Byte;
>   * flower-proto.hh:typedef long long I64;
>   * flower-proto.hh:typedef unsigned char U8;
>   * flower-proto.hh:typedef short I16;
>   * flower-proto.hh:typedef unsigned short U16;
>   * flower-proto.hh:typedef unsigned U32;
>   * flower-proto.hh:typedef int I32;
>   * flower-proto.hh:typedef unsigned long long U64;
> — 
> Dan
> 

Fully +1 - one step closer to eventually ditch flower completely. I
don't see a reason for keeping a separate library which nobody else
uses...

https://codereview.appspot.com/563420043/

Re: automated formatting

[David Kastrup]

David Kastrup  writes:

> Han-Wen Nienhuys  writes:
>
>> 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.
>
>> 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?
> Clang is a pretty big dependency for developers.

I just put up an issue that makes the --sloppy option of fixcc.py work
(but updating to 3.04 does not seem like much of an issue either)

>> Obviously, reformatting code makes patches harder to transport, so
>> we'd have to do it on all active branches at the same time.

That's why I think changing the existing automated formatting style is
an unwanted complication.

In Graham's reign, rerunning the C++ formatter was a pretty frequent
occurence, I think it was more than once yearly.  We also have a Scheme
code formatter that relies on Emacs to do the hard work.

Frankly, I just forgot about either.  I think it would be a good idea to
take up this practice again.  Regarding Clang: unless Astyle falls apart
using C++11 constructs that are going to occur more frequently in
future, I think there is little reason to depart from its use instead of
reverting to Clang.

-- 
David Kastrup

Re: Issue 5698: int->vsize in various alignment and page-layout methods (issue 563420043 by nine.fierce.ball...@gmail.com)

[nine . fierce . ballads]

Reviewers: lemzwerg, hanwenn, dan_faithful.be,


https://codereview.appspot.com/563420043/diff/547500043/lily/grob.cc
File lily/grob.cc (right):

https://codereview.appspot.com/563420043/diff/547500043/lily/grob.cc#newcode492
lily/grob.cc:492: return real_ext;
On 2020/01/27 06:33:09, lemzwerg wrote:
> whitespace

Thank you.  Done in my local copy.

https://codereview.appspot.com/563420043/diff/547500043/lily/include/align-interface.hh
File lily/include/align-interface.hh (right):

https://codereview.appspot.com/563420043/diff/547500043/lily/include/align-interface.hh#newcode37
lily/include/align-interface.hh:37: get_pure_minimum_translations (Grob
*, std::vector const &,
On 2020/01/27 06:33:09, lemzwerg wrote:
> maybe indent this line by two spaces?

That would be inconsistent with the style in other headers (e.g.
smobs.hh, translator.hh).  If this is a readability issue, would adding
a blank line above and below this declaration help?

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

fixes warnings

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

Affected files (+64, -47 lines):
  M lily/align-interface.cc
  M lily/axis-group-interface.cc
  M lily/grob.cc
  M lily/grob-property.cc
  M lily/include/align-interface.hh
  M lily/include/axis-group-interface.hh
  M lily/include/grob.hh
  M lily/include/item.hh
  M lily/include/page-layout-problem.hh
  M lily/item.cc
  M lily/page-layout-problem.cc

Re: Issue 5698: int->vsize in various alignment and page-layout methods (issue 563420043 by nine.fierce.ball...@gmail.com)

[Dan Eble]

On Jan 27, 2020, at 03:26, hanw...@gmail.com wrote:
> 
> LGTM
> 
> Not for this change, but could we do a global 
> 
>  vsize -> size_t
> 
> search & replace.  Do we have a reason to keep our own typedef for this?

I fully support this.  I wasn't going to bring it up yet because I worried that 
it would lead to a lot of discussion and I have bigger fish to fry, but I would 
like to eliminate the following from flower because they reduce clarity.

  * real.hh:typedef double Real;
  * std-string.hh:typedef size_t ssize;
  * std-vector.hh:typedef size_t vsize;

I would like to replace the following with standard types (uint8_t etc.).  The 
standard types are portable, but these are not.

  * flower-proto.hh:typedef unsigned char Byte;
  * flower-proto.hh:typedef long long I64;
  * flower-proto.hh:typedef unsigned char U8;
  * flower-proto.hh:typedef short I16;
  * flower-proto.hh:typedef unsigned short U16;
  * flower-proto.hh:typedef unsigned U32;
  * flower-proto.hh:typedef int I32;
  * flower-proto.hh:typedef unsigned long long U64;
— 
Dan

PATCHES - Countdown for January 27th

[James]

 Hello,

 Here is the current patch countdown list. The next countdown will be on
 January 29th

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

http://philholmes.net/lilypond/allura/ [1]

 
 Push:

No patches to Push at this time.

COUNTDOWN:

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

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

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

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

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

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

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

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

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

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

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

REVIEW:

5682 Only print out open type font substitution if there was a change -
hanwen
 https://sourceforge.net/p/testlilyissues/issues/5682 [23]
http://codereview.appspot.com/577390043 [24] 

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

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

NEW:

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

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

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

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

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

5681 Instrument and improve GC time - hanwen
 https://sourceforge.net/p/testlilyissues/issues/5681 [38]
http://codereview.appspot.com/577380043 [39] 

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

 ***

 Regards,

 James

 

Links:
--
[1] http://philholmes.net/lilypond/allura/
[2] https://sourceforge.net/p/testlilyissues/issues/5695
[3] http://codereview.appspot.com/575530107
[4] https://sourceforge.net/p/testlilyissues/issues/5694
[5] http://codereview.appspot.com/577380046
[6] https://sourceforge.net/p/testlilyissues/issues/5693
[7] http://codereview.appspot.com/547470049
[8] https://sourceforge.net/p/testlilyissues/issues/5689
[9] http://codereview.appspot.com/551390046
[10] https://sourceforge.net/p/testlilyissues/issues/5688
[11] http://codereview.appspot.com/561310045
[12] https://sourceforge.net/p/testlilyissues/issues/5687
[13] http://codereview.appspot.com/547470044
[14] https://sourceforge.net/p/testlilyissues/issues/5686
[15] http://codereview.appspot.com/583390043
[16] https://sourceforge.net/p/testlilyissues/issues/5685
[17] http://codereview.appspot.com/549460043
[18] https://sourceforge.net/p/testlilyissues/issues/5684
[19] http://codereview.appspot.com/547470043
[20] https://sourceforge.net/p/testlilyissues/issues/5680
[21] http://codereview.appspot.com/575540044
[22] https://sourceforge.net/p/testlilyissues/issues/5670
[23] https://sourceforge.net/p/testlilyissues/issues/5682
[24] http://codereview.appspot.com/577390043
[25] https://sourceforge.net/p/testlilyissues/issues/5672
[26] 

Re: automated formatting

[David Kastrup]

Han-Wen Nienhuys  writes:

> 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.

> 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?
Clang is a pretty big dependency for developers.

> If anyone wants to tinker more, see
> https://clang.llvm.org/docs/ClangFormatStyleOptions.html
> for further options.
>
> Obviously, reformatting code makes patches harder to transport, so
> we'd have to do it on all active branches at the same time.

-- 
David Kastrup

Re: session macros

[David Kastrup]

Han-Wen Nienhuys  writes:

> I've been looking at (auto)compilatoin for a bit. It fails with
>
> ;;; WARNING: compilation of
> /home/hanwen/vc/lilypond/lily/share/lilypond/current/scm/chord-entry.scm
> failed:
> ;;; unhandled constant #
>
> I'm puzzled by your macro:
>
> (defmacro define-session (name value)
>   ..
>   (define (add-session-variable name value)
> (set! lilypond-declarations
>   (cons (make-session-variable name value) lilypond-declarations)))
>   `(,add-session-variable ',name ,value))
>
> I understand why we need sessions, but I don't understand why this has
> to be a macro, and why the macro has to define add-session-variable.
>
> Why can't this be something like
>
>   (define lilypond-declarations '())
>   (define (define-session name value)
>  (set! lilypond-declarations
>   (cons (make-session-variable name value) lilypond-declarations)))
>
>
> 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.

-- 
David Kastrup

Re: automated formatting

[Werner LEMBERG]

> 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.

I like that, thanks!

> The current config modifies about 11k lines, mostly because of
> different line breaks (necessary to keep the 80 column limit.)

As soon as I'm able to run this, I will check the formatting in
detail.

The 80 column limit is a special beast.  Sometimes it *does* make
sense to not follow this restriction, especially if the excess is just
a few characters...

> Obviously, reformatting code makes patches harder to transport, so
> we'd have to do it on all active branches at the same time.

I suggest to do such a clean-up after the releases of both 2.20 and
2.21.


Werner

Add a tentative .clang-format for LilyPond. (issue 561340043 by hanw...@gmail.com)

[lemzwerg--- via Discussions on LilyPond development]

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.

https://codereview.appspot.com/561340043/

session macros

[Han-Wen Nienhuys]

I've been looking at (auto)compilatoin for a bit. It fails with

;;; WARNING: compilation of
/home/hanwen/vc/lilypond/lily/share/lilypond/current/scm/chord-entry.scm
failed:
;;; unhandled constant #

I'm puzzled by your macro:

(defmacro define-session (name value)
  ..
  (define (add-session-variable name value)
(set! lilypond-declarations
  (cons (make-session-variable name value) lilypond-declarations)))
  `(,add-session-variable ',name ,value))

I understand why we need sessions, but I don't understand why this has
to be a macro, and why the macro has to define add-session-variable.

Why can't this be something like

  (define lilypond-declarations '())
  (define (define-session name value)
 (set! lilypond-declarations
  (cons (make-session-variable name value) lilypond-declarations)))


When calling the macro twice, we'll define add-session-variable twice,
which seems fishy.

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

Re: GUILE2: generate internals doc in UTF-8 (issue 575540045 by hanw...@gmail.com)

[dak]

On 2020/01/27 08:23:41, hanwenn wrote:
> On 2020/01/24 11:15:10, hanwenn wrote:
> >
>
https://codereview.appspot.com/575540045/diff/571410043/scm/documentation-generate.scm
> > File scm/documentation-generate.scm (right):
> > 
> >
>
https://codereview.appspot.com/575540045/diff/571410043/scm/documentation-generate.scm#newcode98
> > scm/documentation-generate.scm:98: (if (defined?
'set-port-encoding!)
> > On 2020/01/24 11:08:51, dak wrote:
> > > I think we usually do this as
> > > 
> > > (if (guile-v2) ...
> > > 
> > > in order to better be able to find dependencies and ultimately
(after
> > migration)
> > > throw them out.  Also I am not sure that the byte-compiler is
happy about
> > those
> > > constructs.
> > 
> > Done.
> 
> ping.

I now set status of issue #5683 back to Patch-new to trigger retesting. 
If you submit followup patches via git-cl, that should happen
automatically.

https://codereview.appspot.com/575540045/

GUILE v2: set postscript output port to Latin1 (issue 563400065 by hanw...@gmail.com)

[lemzwerg--- via Discussions on LilyPond development]

LGTM

https://codereview.appspot.com/563400065/

automated formatting

[Han-Wen Nienhuys]

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?

The current config modifies about 11k lines, mostly because of
different line breaks (necessary to keep the 80 column limit.)

If anyone wants to tinker more, see
https://clang.llvm.org/docs/ClangFormatStyleOptions.html
for further options.

Obviously, reformatting code makes patches harder to transport, so
we'd have to do it on all active branches at the same time.

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

UTF-8 and default locale

[Han-Wen Nienhuys]

Hey David,

before we go into formal review, could you have a look at

https://github.com/hanwen/lilypond/commit/3402c61aa0d6f36b8dce984947d61ba01b0a6350

The commits fix Unicode lyrics,  but maybe we want to change the
setlocale() calls in main instead.

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

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

[hanwenn]

Reviewers: ,

Message:
this is preparation for
https://github.com/hanwen/lilypond/commit/599878a08c18323810aaa1dee3bf4d9b208e223c

Description:
Clean up embedded scheme parsing/evaluation.

Renames and reorders functions to clarify the mechanism. No functional
changes.

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

Affected files (+88, -87 lines):
  M lily/include/parse-scm.hh
  M lily/lexer.ll
  M lily/parse-scm.cc
  M lily/undead.cc

Re: Instrument and improve GC time (issue 577380043 by hanw...@gmail.com)

[hanwenn]

dup issue.

https://codereview.appspot.com/577380043/

Re: In verbose mode, dump time spent on parsing lily.scm and friends. (issue 573420043 by hanw...@gmail.com)

[hanwenn]

Reviewers: lemzwerg,

Message:
done.

Description:
In verbose mode, dump time spent on parsing lily.scm and friends.

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

Affected files (+6, -6 lines):
  M lily/guile-init.cc
  M lily/main.cc


Index: lily/guile-init.cc
diff --git a/lily/guile-init.cc b/lily/guile-init.cc
index 
553b7c1119d17ff3acda52d49fc581a97062caa8..b25b8bbfccaa5678188a23e977ac5a57d1b3d24a
 100644
--- a/lily/guile-init.cc
+++ b/lily/guile-init.cc
@@ -18,6 +18,8 @@
   along with LilyPond.  If not, see .
 */
 
+#include "cpu-timer.hh"
+#include "international.hh"
 #include "lily-guile.hh"
 #include "main.hh"
 #include "warn.hh"
@@ -56,6 +58,7 @@ ly_init_ly_module ()
   for (vsize i = scm_init_funcs_->size (); i--;)
 (scm_init_funcs_->at (i)) ();
 
+  Cpu_timer timer;
   if (is_loglevel (LOG_DEBUG))
 {
   debug_output ("[", true);
@@ -65,6 +68,7 @@ ly_init_ly_module ()
 }
 
   scm_primitive_load_path (scm_from_ascii_string ("lily.scm"));
+  debug_output (_f ("Load lily.scm: %.2f seconds", timer.read ()));
 }
 
 void
Index: lily/main.cc
diff --git a/lily/main.cc b/lily/main.cc
index 
bc86143a4781d58a86077132a920464091cb88cc..60b5738265e6d9a41f9ad2dcfecf887c823de01c
 100644
--- a/lily/main.cc
+++ b/lily/main.cc
@@ -515,6 +515,7 @@ main_with_guile (void *, int, char **)
   // SCM result = scm_call_1 (
   //  scm_variable_ref (call_with_error_handling),
   //  scm_call_1 (ly_lily_module_constant 
("lilypond-main"), files));
+
   Lily::lilypond_main (files);
 
   /* Unreachable.  */
@@ -743,9 +744,6 @@ setup_guile_gc_env ()
"104857600", overwrite);
 }
 
-#if (GUILEV2)
-extern unsigned long GC_free_space_divisor;
-#endif
 
 void
 setup_guile_v2_env ()
@@ -763,7 +761,6 @@ setup_guile_v2_env ()
   */
  sane_putenv("XDG_CACHE_HOME",
   lilypond_datadir, true);
- GC_free_space_divisor = 1;
 }
 
 void
@@ -773,10 +770,9 @@ setup_guile_env ()
  */
 {
 
+  setup_guile_gc_env ();  // configure garbage collector
 #if (GUILEV2)
   setup_guile_v2_env ();  // configure Guile V2 behaviour
-#else
-  setup_guile_gc_env ();  // configure garbage collector
 #endif
 }
 

Re: Provide --loglevel=PROGRESS for make VERBOSE=1 (issue 575540044 by hanw...@gmail.com)

[hanwenn]

Reviewers: Dan Eble,

Message:
On 2020/01/27 02:52:44, Dan Eble wrote:
> > This provides enough information to diagnose problems in included
> > files while building the documentation.
> 
> LGTM, though I wonder why these problems (whatever they are) do not
trigger
> useful diagnostic messages at WARN or higher levels, which would be
seen even at
> the default verbosity level.  In other words, is this change just
working around
> deficiencies of the program?

kind of. If there is a problem with an input file, the progress messages
would tell you what
the last file processed was. 

The clean way to do this, is to catch the exception in python, annotate
it with the current file
and rethrow. It's more work, and this gets it done well enough for
debugging.

Description:
Provide --loglevel=PROGRESS for make VERBOSE=1

This provides enough information to diagnose problems in included
files while building the documentation.

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

Affected files (+4, -0 lines):
  M make/lilypond-vars.make


Index: make/lilypond-vars.make
diff --git a/make/lilypond-vars.make b/make/lilypond-vars.make
index 
2274e173ba548c2079a160439c649fae455909d0..66b64b52496f9be96925586dad29fb6fafbeffe0
 100644
--- a/make/lilypond-vars.make
+++ b/make/lilypond-vars.make
@@ -56,7 +56,11 @@ endif
 ifdef SILENT
   LILYPOND_BOOK_WARN = --loglevel=WARN
 else
+ ifdef VERBOSE
+  LILYPOND_BOOK_WARN = --loglevel=PROGRESS
+ else
   LILYPOND_BOOK_WARN = --loglevel=BASIC
+ endif
 endif
 
 LILYPOND_BOOK_INFO_IMAGES_DIR = $(if 
$(INFO_IMAGES_DIR),--info-images-dir=$(INFO_IMAGES_DIR),)

Re: Issue 5698: int->vsize in various alignment and page-layout methods (issue 563420043 by nine.fierce.ball...@gmail.com)

[hanwenn]

LGTM

Not for this change, but could we do a global 


  vsize -> size_t

search & replace.  Do we have a reason to keep our own typedef for this?

https://codereview.appspot.com/563420043/

Re: GUILE2: generate internals doc in UTF-8 (issue 575540045 by hanw...@gmail.com)

[hanwenn]

On 2020/01/24 11:15:10, hanwenn wrote:
>
https://codereview.appspot.com/575540045/diff/571410043/scm/documentation-generate.scm
> File scm/documentation-generate.scm (right):
> 
>
https://codereview.appspot.com/575540045/diff/571410043/scm/documentation-generate.scm#newcode98
> scm/documentation-generate.scm:98: (if (defined? 'set-port-encoding!)
> On 2020/01/24 11:08:51, dak wrote:
> > I think we usually do this as
> > 
> > (if (guile-v2) ...
> > 
> > in order to better be able to find dependencies and ultimately
(after
> migration)
> > throw them out.  Also I am not sure that the byte-compiler is happy
about
> those
> > constructs.
> 
> Done.

ping.

https://codereview.appspot.com/575540045/