Re: Issue 5712: Split context_specification syntax constructor (issue 565560043 by nine.fierce.ball...@gmail.com)
LGTM https://codereview.appspot.com/565560043/
Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)
https://codereview.appspot.com/577450043/diff/579260043/lily/main.cc File lily/main.cc (right): https://codereview.appspot.com/577450043/diff/579260043/lily/main.cc#newcode174 lily/main.cc:174: /* x86 defaults to using 80-bit extended precision arithmetic. This can cause On 2020/01/29 23:54:27, Dan Eble wrote: > I missed this comment the first time around because Rietveld hid it. This > explains the situation well. Well, this comment was not part of current patch https://codereview.appspot.com/577450043/
Issue 5712: Split context_specification syntax constructor (issue 565560043 by nine.fierce.ball...@gmail.com)
Reviewers: ,
Description:
https://sourceforge.net/p/testlilyissues/issues/5712/
... into context_create and context_find_or_create. This trades the
obscurity of a boolean constant for the clarity of specific names.
Please review this at https://codereview.appspot.com/565560043/
Affected files (+13, -9 lines):
M lily/include/lily-imports.hh
M lily/lily-imports.cc
M lily/parser.yy
M scm/ly-syntax-constructors.scm
Index: lily/include/lily-imports.hh
diff --git a/lily/include/lily-imports.hh b/lily/include/lily-imports.hh
index
1dbafaab8cf813b3944ce9ca240bbbc76d2dd8fe..42573978d2feb1467a2684654084d9c608bd34e7
100644
--- a/lily/include/lily-imports.hh
+++ b/lily/include/lily-imports.hh
@@ -126,7 +126,8 @@ namespace Syntax {
extern Variable argument_error;
extern Variable composed_markup_list;
extern Variable context_change;
- extern Variable context_specification;
+ extern Variable context_create;
+ extern Variable context_find_or_create;
extern Variable create_script;
extern Variable create_script_function;
extern Variable event_chord;
Index: lily/lily-imports.cc
diff --git a/lily/lily-imports.cc b/lily/lily-imports.cc
index
9cee91b5d1d5ff26d2331db5d062e96109c0d92b..42bae16501c45feb09f8c4033a88557320bdfb52
100644
--- a/lily/lily-imports.cc
+++ b/lily/lily-imports.cc
@@ -118,7 +118,8 @@ namespace Syntax {
Variable argument_error ("argument-error");
Variable composed_markup_list ("composed-markup-list");
Variable context_change ("context-change");
- Variable context_specification ("context-specification");
+ Variable context_create ("context-create");
+ Variable context_find_or_create ("context-find-or-create");
Variable create_script ("create-script");
Variable create_script_function ("create-script-function");
Variable event_chord ("event-chord");
Index: lily/parser.yy
diff --git a/lily/parser.yy b/lily/parser.yy
index
dd58232ed12343508bc4ba55ab412c26879c32cf..89511c5d9d67a8091cbf9e043d9b99def124be58
100644
--- a/lily/parser.yy
+++ b/lily/parser.yy
@@ -1605,10 +1605,10 @@ context_mod_list:
context_prefix:
CONTEXT symbol optional_id optional_context_mods {
- $$ = START_MAKE_SYNTAX (context_specification, $2, $3, $4,
SCM_BOOL_F);
+ $$ = START_MAKE_SYNTAX (context_find_or_create, $2, $3, $4);
}
| NEWCONTEXT symbol optional_id optional_context_mods {
- $$ = START_MAKE_SYNTAX (context_specification, $2, $3, $4,
SCM_BOOL_T);
+ $$ = START_MAKE_SYNTAX (context_create, $2, $3, $4);
}
;
@@ -2626,7 +2626,7 @@ mode_changed_music:
parser->lexer_->pop_state ();
}
| mode_changing_head_with_context optional_context_mods
grouped_music_list {
- $$ = MAKE_SYNTAX (context_specification, @$, $1, SCM_EOL, $2,
SCM_BOOL_T, $3);
+ $$ = MAKE_SYNTAX (context_create, @$, $1, SCM_EOL, $2, $3);
if (scm_is_eq ($1, ly_symbol2scm ("ChordNames")))
{
$$ = MAKE_SYNTAX (unrelativable_music, @$, $$);
Index: scm/ly-syntax-constructors.scm
diff --git a/scm/ly-syntax-constructors.scm b/scm/ly-syntax-constructors.scm
index
c1950c4d3e4ad64ddf80960d0a731e7f4e3812b5..4303ff4fd2135c7599cf43f761b70d86b3d2c498
100644
--- a/scm/ly-syntax-constructors.scm
+++ b/scm/ly-syntax-constructors.scm
@@ -209,12 +209,14 @@ into a @code{MultiMeasureTextEvent}."
'duration duration
'elements articulations)))
-(define-public (context-specification type id ops create-new mus)
- (let ((csm (context-spec-music mus type id)))
-(set! (ly:music-property csm 'property-operations) ops)
-(if create-new (set! (ly:music-property csm 'create-new) #t))
+(define-public (context-create type id ops mus)
+ (let ((csm (context-spec-music mus type id ops)))
+(set! (ly:music-property csm 'create-new) #t)
(ly:set-origin! csm)))
+(define-public (context-find-or-create type id ops mus)
+ (ly:set-origin! (context-spec-music mus type id ops)))
+
(define-public (composed-markup-list commands markups)
;; `markups' being a list of markups, eg (markup1 markup2 markup3),
;; and `commands' a list of commands with their scheme arguments, in reverse
order,
Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)
https://codereview.appspot.com/577450043/diff/579260043/lily/main.cc File lily/main.cc (right): https://codereview.appspot.com/577450043/diff/579260043/lily/main.cc#newcode174 lily/main.cc:174: /* x86 defaults to using 80-bit extended precision arithmetic. This can cause I missed this comment the first time around because Rietveld hid it. This explains the situation well. https://codereview.appspot.com/577450043/diff/579260043/lily/main.cc#newcode210 lily/main.cc:210: "\n mov $0x027f, %eax" It would be very nice if this magic number and the one in line 186 were not separately defined. https://codereview.appspot.com/577450043/
Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)
On 2020/01/29 22:36:03, Carl wrote: > While this answer is specific, it could be clearer, IMO: > > Reviewing the Intel Manuals at > https://software.intel.com/sites/default/files/managed/a4/60/253665-sdm-vol-1.pdf > Section 4.2.2. > > We can see that the 64 bit significand of Double Extended Precision refers to an > 80-bit floating point representation, and the 53-bit significand of Double > Precision refers to a 64-bit floating point representation. > > I think if we just leave the 64/53 bit numbers in the comment, the reader might > think we are not using 64-bit floating-point representations. Carl, I'm shepherding this patch for Arnold. I'll point him to the review-comments, can't say anything about it myself. https://codereview.appspot.com/577450043/
Doc: NR - 2.10.2 - Arabic Music - improved example (issue 572600048 by pkxgnugi...@runbox.com)
https://codereview.appspot.com/572600048/diff/548660043/Documentation/snippets/new/non-traditional-key-signatures.ly
File Documentation/snippets/new/non-traditional-key-signatures.ly
(right):
https://codereview.appspot.com/572600048/diff/548660043/Documentation/snippets/new/non-traditional-key-signatures.ly#newcode5
Documentation/snippets/new/non-traditional-key-signatures.ly:5:
version-specific, non-traditional, world-music"
Breaking this line breaks makelsr.py . How did this get through?
I see that it was silently fixed in
commit c511518a4e7669ac7263f8a28565debefe25a358
Author: James Lowe
Date: Mon Apr 15 21:03:57 2019 +0100
makelsr run for previous commit
Changes made based on commit
Doc: NR - 2.10.2 - Arabic Music - improved example
03f78e3e7bda039ea08744d3736bfa4a39dea2a4
diff --git
a/Documentation/snippets/new/non-traditional-key-signatures.ly
b/Documentation/snippets/new/non-traditional-key-signatures.ly
index f5967009bd..f2a16ca841 100644
--- a/Documentation/snippets/new/non-traditional-key-signatures.ly
+++ b/Documentation/snippets/new/non-traditional-key-signatures.ly
@@ -1,8 +1,7 @@
\version "2.19.21"
\header {
- lsrtags = "contemporary-notation, pitches, staff-notation,
- version-specific, non-traditional, world-music"
+ lsrtags = "contemporary-notation, pitches, staff-notation,
version-specific, non-traditional, world-music"
which is a really, really, really bad idea since obviously makelsr runs
are not cherry-picked, so this fix should have been made in a separate
commit. Took me a few hours (after all, make doc will take about half
an hour on my computer). Please folks, don't pack corrective commits
into the commits running scripts.
https://codereview.appspot.com/572600048/
Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)
While this answer is specific, it could be clearer, IMO: Reviewing the Intel Manuals at https://software.intel.com/sites/default/files/managed/a4/60/253665-sdm-vol-1.pdf Section 4.2.2. We can see that the 64 bit significand of Double Extended Precision refers to an 80-bit floating point representation, and the 53-bit significand of Double Precision refers to a 64-bit floating point representation. I think if we just leave the 64/53 bit numbers in the comment, the reader might think we are not using 64-bit floating-point representations. https://codereview.appspot.com/577450043/
Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)
The comment could be more helpful. Explaining what this code does is more important than explaining that it replaces something that no longer exists. I've found this documentation on the x87 FPU Control Word: http://home.agh.edu.pl/~amrozek/x87.pdf Section 8.1.4. It looks like the value 0x027f changes the Precision Control (PC) field from the default of "Double Extended Precision (64 bits)" (value 3) to "Double Precision (53 bits)" (value 2). https://codereview.appspot.com/577450043/
Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)
Reviewers: ,
Description:
Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries
Issue 4943
As Issue 4943 on Windows compilation was triggered by
missing _FPU_SETCW() in the MINGW libraries, an alternate call to
initiate the X87 FPU setup as an inline-assembler command is added
- for MINGW 32 Bit compilation only.
Please review this at https://codereview.appspot.com/577450043/
Affected files (+24, -0 lines):
M lily/main.cc
Index: lily/main.cc
diff --git a/lily/main.cc b/lily/main.cc
index
4834ff82aa9b6bb649bfe8fbe863877b56c819d1..cfb04d86f8d0b200c3a6e15ae8a36960b31570fa
100644
--- a/lily/main.cc
+++ b/lily/main.cc
@@ -192,6 +192,30 @@ configure_fpu ()
static void
configure_fpu ()
{
+#if ((defined (__x86__) || defined (__i386__)) \
+ && defined (__MINGW32__) && defined (__code_model_32__) &&
!defined(__SSE2_MATH__))
+ /* If this is a MINGW compilation (for Windows),
+ * but not using SSE2 arithmetic unit nor is a 64 bit compilation (which
uses SSE2 by default)
+ * _FPU_SETCW() got lost in the MINGW libraries!
+ * Here is an inline assembler call to execute the same task for the X87
arithmetic unit.
+ *
+ * TODO: output a message what we're going to do here, _only_ if DEBUG level
is selected,
+ * but configure_fpu() is called before the commandline options get
evaluated.
+ * At the moment the info message will be blanked on the console, but
if output is
+ * directed into a file, most editors will show it.
+ */
+ fprintf(stderr, " X87 FPU setup via asm() ... ");
+ asm(
+ " push %ebp"
+"\n mov $0x027f, %eax"
+"\n push %eax"
+"\n mov %esp, %ebp"
+"\n fldcw (%ebp)"
+"\n pop %eax"
+"\n pop %ebp"
+ );
+ fprintf(stderr, "done.\r \r");
+#endif /* (defined(__x86__) || defined(__i386__)) && defined (__MINGW32__) &&
defined (__code_model_32__) && !defined(__SSE2_MATH__) */
}
#endif /* defined(__x86__) || defined(__i386__) */
Re: 2.91.84 - windows-only bugs
Am Mi., 29. Jan. 2020 um 16:41 Uhr schrieb Dan Eble : > > > > > On Jan 29, 2020, at 10:17, Masamichi Hosoda wrote: > > > >> I ask because, in the german forum Arnold found a method to cure some > >> windows-only bugs., about mis-predicted force and probably several > >> assertion-failures: > >> https://lilypondforum.de/index.php/topic,609.msg3463.html#msg3463 > > > > The patch is very interesting. > > + fprintf(stderr, " X87 FPU setup via asm() ... "); > + asm( > + " push %ebp" > +"\n mov $0x027f, %eax" > +"\n push %eax" > +"\n mov %esp, %ebp" > +"\n fldcw (%ebp)" > +"\n pop %eax" > +"\n pop %ebp" > + ); > + fprintf(stderr, "done.\r \r"); > > What does this do? Is it something that could be handled portably with these > C++11 functions? > https://en.cppreference.com/w/cpp/numeric/fenv > — > Dan > I've not the slightest idea... To be more verbose, in the german forum Arnold posted about the problem. While I couldn't help directly, I offered to make special windows-installers via GUB for testing/diagnostic purposes and for the found fix. That's my whole part in the story. Well, I'll shephard the patch as well and probably forward communications, if Arnold can't do himself. In generally we release our windows and mac version untested. Thus I thought it's the least I can do, if someone undertakes debugging/fixing a windows-only problem. Best, Harm
Re: 2.91.84 - windows-only bugs
Am Mi., 29. Jan. 2020 um 16:17 Uhr schrieb Masamichi Hosoda : > > > I ask because, in the german forum Arnold found a method to cure some > > windows-only bugs., about mis-predicted force and probably several > > assertion-failures: > > https://lilypondforum.de/index.php/topic,609.msg3463.html#msg3463 > > The patch is very interesting. > > I've tried x87mant53.dll showed in the german forum. > It solved not only Issue 4943 but also Issue 4975. > > I guess that these issues do not only occur on Windows, > but on all x86 32 bit platforms except Linux. > Therefore, I think that `defined (__MINGW32__)` is not necessary. I'm not the author of the patch, but will point Arnold to it. Best, Harm
Re: 2.91.84 - windows-only bugs
Am Mi., 29. Jan. 2020 um 13:21 Uhr schrieb David Kastrup : > > Thomas Morley writes: > > > Hi all, > > > > iirc in Salzburg there was the plan to do a 2.19.84-release, soon > > followed by stable 2.20 > > May I ask how's the state of 2.19.84? > > I just got news from Phil (who had been offline for a while) and he > plans to set to it this weekend, time allowing. > > > I ask because, in the german forum Arnold found a method to cure some > > windows-only bugs., about mis-predicted force and probably several > > assertion-failures: > > https://lilypondforum.de/index.php/topic,609.msg3463.html#msg3463 > > > > Though, this would first need to be reviewed and put in the source > > and then been tested by users. > > For the latter, I made a windows-installer containing this patch and > > could post about it on -user for testing. > > > > Alternatively, we could postpone it, implementing it in 2.21 > > > > Thoughts? > > After 2.19.84, we stipulated a two-week window until 2.20. I'd propose > that you try getting the respective patch into review right now if it is > ok for you, and we aim for its appearance in 2.19.84 (by being as rushed > about including it and cherry-picking into the stable branch as > appropriate, and, if push comes to shove, giving a slight delay to > 2.19.84). This is kind of an important thing to have fixed in the > stable release if we see a chance of doing so, and having that two-week > window would be a good thing. Patch is up, https://codereview.appspot.com/577450043 added to https://sourceforge.net/p/testlilyissues/issues/4943/ > I haven't yet looked at the patch/discussion yet: this is just my first > reaction rather than an LGTM. Understood Thanks, Harm
Re: Issue 5362: Generalize context searches (Take 2) (issue 579250043 by nine.fierce.ball...@gmail.com)
https://codereview.appspot.com/579250043/
Re: 2.91.84 - windows-only bugs
> On Jan 29, 2020, at 10:17, Masamichi Hosoda wrote: > >> I ask because, in the german forum Arnold found a method to cure some >> windows-only bugs., about mis-predicted force and probably several >> assertion-failures: >> https://lilypondforum.de/index.php/topic,609.msg3463.html#msg3463 > > The patch is very interesting. + fprintf(stderr, " X87 FPU setup via asm() ... "); + asm( + " push %ebp" +"\n mov $0x027f, %eax" +"\n push %eax" +"\n mov %esp, %ebp" +"\n fldcw (%ebp)" +"\n pop %eax" +"\n pop %ebp" + ); + fprintf(stderr, "done.\r \r"); What does this do? Is it something that could be handled portably with these C++11 functions? https://en.cppreference.com/w/cpp/numeric/fenv — Dan
Re: Doc: Correct and extend infos about LilyDev setup (issue 561360043 by michael.kaepp...@googlemail.com)
LGTM Just two thoughts: 1. I'm not sure why you suggest how to set the root password, as the dev user can get admin privileges via sudo. 2. While keyboard configuration is a step needed for any non-US contributor, reconfiguring the locales should be useful only for translators, unless I miss something. Perhaps you could say that these two configurations are optional. https://codereview.appspot.com/561360043/
Re: 2.91.84 - windows-only bugs
> I ask because, in the german forum Arnold found a method to cure some > windows-only bugs., about mis-predicted force and probably several > assertion-failures: > https://lilypondforum.de/index.php/topic,609.msg3463.html#msg3463 The patch is very interesting. I've tried x87mant53.dll showed in the german forum. It solved not only Issue 4943 but also Issue 4975. I guess that these issues do not only occur on Windows, but on all x86 32 bit platforms except Linux. Therefore, I think that `defined (__MINGW32__)` is not necessary.
Re: Move Guile-style modules from scm to scm-modules (issue 567140045 by m...@ursliska.de)
Am Mittwoch, den 29.01.2020, 07:01 -0800 schrieb d...@gnu.org: > On 2020/01/29 12:20:10, mail5 wrote: > > Unfortunately I haven't set up a build system on my new computer > > yet, > so this > > patch is not tested locally at all, so I'm humbly waiting for the > automated > > tests to succeed or fail ... > > I don't like the "use-modules clauses adjusted accordingly". I don't > think it makes sense readjusting use-modules clauses all the time > while > we are deciding on the final module organisation, so I'd strongly > suggest first biting the bullet and deciding on a syntax for a > user-level command able to load Scheme modules without further > options, > and then introduce that command. In that manner, future directory > organisations (which are almost certain to come) will not affect the > source of user-level documents any more. > > https://codereview.appspot.com/567140045/ Maybe I'm missing something, but AFAICS there will always be the need for a module path like (ice-9 regex), or (scm display-lily). We will have that with *any* user-facing load syntax. My thought was to separate the two different types of .scm files in that directory, and that could of course also be achieved by moving the *other* files, those that are loaded with ly:load from lily.scm to a different directory. Or - of course - I can simply drop this and add new modules to that same directory for now. >
Re: Move Guile-style modules from scm to scm-modules (issue 567140045 by m...@ursliska.de)
On 2020/01/29 12:20:10, mail5 wrote: > Unfortunately I haven't set up a build system on my new computer yet, so this > patch is not tested locally at all, so I'm humbly waiting for the automated > tests to succeed or fail ... I don't like the "use-modules clauses adjusted accordingly". I don't think it makes sense readjusting use-modules clauses all the time while we are deciding on the final module organisation, so I'd strongly suggest first biting the bullet and deciding on a syntax for a user-level command able to load Scheme modules without further options, and then introduce that command. In that manner, future directory organisations (which are almost certain to come) will not affect the source of user-level documents any more. https://codereview.appspot.com/567140045/
Move Guile-style modules from scm to scm-modules (issue 567140045 by m...@ursliska.de)
Reviewers: , Message: Unfortunately I haven't set up a build system on my new computer yet, so this patch is not tested locally at all, so I'm humbly waiting for the automated tests to succeed or fail ... Description: Move Guile-style modules from scm to scm-modules As an initial step towards a package mechanism this patch separates .scm files in /scm that are directly loaded with ly:load from those that are explicitly loaded with (use-modules (scm ... The latter are moved to a new directory /scm-modules, and the (use-modules clauses adjusted accordingly. Load Scheme modules through scm-modules Move Scheme modules to scm-modules Please review this at https://codereview.appspot.com/567140045/ Affected files (+83, -7862 lines): M Documentation/snippets/accordion-register-symbols.ly M input/regression/display-lily-tests.ly M input/regression/scheme-book-scores.ly M input/regression/to-xml.ly M ly/articulate.ly M ly/festival.ly M ly/graphviz-init.ly M ly/guile-debugger.ly M ly/init.ly A + scm-modules/accreg.scm A + scm-modules/clip-region.scm A + scm-modules/coverage.scm A + scm-modules/display-lily.scm A + scm-modules/editor.scm A + scm-modules/framework-eps.scm A + scm-modules/framework-null.scm A + scm-modules/framework-ps.scm A + scm-modules/framework-scm.scm A + scm-modules/framework-svg.scm A + scm-modules/graphviz.scm A + scm-modules/guile-debugger.scm A + scm-modules/lily.scm A + scm-modules/ly-syntax-constructors.scm A + scm-modules/memory-trace.scm A + scm-modules/output-ps.scm A + scm-modules/output-socket.scm A + scm-modules/output-svg.scm A + scm-modules/page.scm A + scm-modules/paper-system.scm A + scm-modules/ps-to-png.scm A + scm-modules/safe-utility-defs.scm A + scm-modules/song.scm A + scm-modules/song-util.scm A + scm-modules/to-xml.scm D scm/accreg.scm M scm/backend-library.scm M scm/chord-entry.scm D scm/clip-region.scm D scm/coverage.scm M scm/define-music-display-methods.scm M scm/define-music-types.scm M scm/define-note-names.scm D scm/display-lily.scm M scm/document-markup.scm M scm/documentation-generate.scm D scm/editor.scm D scm/framework-eps.scm D scm/framework-null.scm D scm/framework-ps.scm D scm/framework-scm.scm M scm/framework-socket.scm D scm/framework-svg.scm D scm/graphviz.scm D scm/guile-debugger.scm D scm/lily.scm M scm/lily-library.scm D scm/ly-syntax-constructors.scm D scm/memory-trace.scm M scm/music-functions.scm D scm/output-ps.scm D scm/output-socket.scm D scm/output-svg.scm D scm/page.scm M scm/paper.scm D scm/paper-system.scm D scm/ps-to-png.scm D scm/safe-utility-defs.scm D scm/song.scm D scm/song-util.scm M scm/stencil.scm D scm/to-xml.scm M scripts/lilypond-invoke-editor.scm
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
https://codereview.appspot.com/577410045/diff/551410043/lily/parse-scm.cc File lily/parse-scm.cc (right): https://codereview.appspot.com/577410045/diff/551410043/lily/parse-scm.cc#newcode59 lily/parse-scm.cc:59: Pouring oil on the fire... Rietveld highlighting indicates non-empty lines consisting only of spaces. Incidentally, those would already get highlighted with git log --check https://codereview.appspot.com/577410045/
Re: 2.91.84 - windows-only bugs
Thomas Morley writes: > Hi all, > > iirc in Salzburg there was the plan to do a 2.19.84-release, soon > followed by stable 2.20 > May I ask how's the state of 2.19.84? I just got news from Phil (who had been offline for a while) and he plans to set to it this weekend, time allowing. > I ask because, in the german forum Arnold found a method to cure some > windows-only bugs., about mis-predicted force and probably several > assertion-failures: > https://lilypondforum.de/index.php/topic,609.msg3463.html#msg3463 > > Though, this would first need to be reviewed and put in the source > and then been tested by users. > For the latter, I made a windows-installer containing this patch and > could post about it on -user for testing. > > Alternatively, we could postpone it, implementing it in 2.21 > > Thoughts? After 2.19.84, we stipulated a two-week window until 2.20. I'd propose that you try getting the respective patch into review right now if it is ok for you, and we aim for its appearance in 2.19.84 (by being as rushed about including it and cherry-picking into the stable branch as appropriate, and, if push comes to shove, giving a slight delay to 2.19.84). This is kind of an important thing to have fixed in the stable release if we see a chance of doing so, and having that two-week window would be a good thing. I haven't yet looked at the patch/discussion yet: this is just my first reaction rather than an LGTM. -- David Kastrup
Re: Add a tentative .clang-format for LilyPond. (issue 561340043 by hanw...@gmail.com)
On 2020/01/29 07:41:40, lemzwerg wrote: > The output looks good. BTW, for the sake of Emacs, it would be nice if two > spaces after a full dot could be retained in comments. Does such an option > exist? Retaining everything with ReflowComments:false might be the only way. https://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormatStyleOptions.html https://codereview.appspot.com/561340043/
Re: Announce end of multi-measure-rest (issue 561310045 by hanw...@gmail.com)
LGTM. Excuse my Phone spellcheck From: lilypond-devel on behalf of hanw...@gmail.com Sent: Tuesday, January 28, 2020 10:59:08 PM To: lemzw...@googlemail.com ; nine.fierce.ball...@gmail.com ; rietveld...@tutanota.com Cc: re...@codereview-hr.appspotmail.com ; lilypond-devel@gnu.org Subject: Re: Announce end of multi-measure-rest (issue 561310045 by hanw...@gmail.com) ping? 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 06:06:15, dak wrote: > Oh, I am sorry to have been misleading with my comment forgiven https://codereview.appspot.com/563430046/
Re: GUILE 2: Increase initial heap (issue 547540043 by hanw...@gmail.com)
On 2020/01/29 10:24:12, lemzwerg wrote: > > Is there ever a reason for the user to change the value? Say, to speed up testing? https://codereview.appspot.com/547540043/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/29 06:36:07, hanwenn wrote: > > 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 Errors mean non-intentional things. My own take here is that we would not want to use references on things that may be used as SCM values, so things like Grob = *unsmob (sbla); would be quite undesirable. However, for code that has no Schemish implications, I find it perfectly fine to use C++ references in the manner they are intended to be used. There has been a recent push to settle on C++11 as a programming standard to adhere to in order to be more friendly to newcomers, and it seems sort of pointless to promote C89 standards to go alongside. That makes people less, not more confident in what they may rely on using. > grep --color -nH -e '&' lily/include/*h|grep -v const > > Also, it is rare to check incoming parameters for nullness in implementation. I am not exactly sure I'd call that a feature: we had crashes because of it. Some trampolining code now has the requisite assertions inside since one cannot perform those checks too late: GCC optimises checks like "if (!this)" away these days. https://codereview.appspot.com/577410045/
2.91.84 - windows-only bugs
Hi all, iirc in Salzburg there was the plan to do a 2.19.84-release, soon followed by stable 2.20 May I ask how's the state of 2.19.84? I ask because, in the german forum Arnold found a method to cure some windows-only bugs., about mis-predicted force and probably several assertion-failures: https://lilypondforum.de/index.php/topic,609.msg3463.html#msg3463 Though, this would first need to be reviewed and put in the source and then been tested by users. For the latter, I made a windows-installer containing this patch and could post about it on -user for testing. Alternatively, we could postpone it, implementing it in 2.21 Thoughts? Cheers, Harm
Re: GUILE 2: Increase initial heap (issue 547540043 by hanw...@gmail.com)
LGTM
https://codereview.appspot.com/547540043/diff/561380043/lily/main.cc
File lily/main.cc (right):
https://codereview.appspot.com/547540043/diff/561380043/lily/main.cc#newcode772
lily/main.cc:772: sane_putenv("GC_INITIAL_HEAP_SIZE", "40M", false);
Do you think it makes sense to mention this environment variable in the
lilypond documentation? Is there ever a reason for the user to change
the value?
https://codereview.appspot.com/547540043/
Visibility of files loaded with ly:load, location of "local" guile modules
One final question before I start working on some package code proposal: All the basic Scheme files are loaded in lily.scm with (for-each ly:load init-scheme-files) What does this do internally? I.e. how is the visibility handled of definitions/bindings from names in files loaded with ly:load ? When I manually ly:load a .scm file I can see both variables created with define and define-public. I have the impression that everything loaded with ly:load is visible to any .ly file afterwards, is that correct? So my approach would be to create a file packages.scm, add it to the list of auto-loaded files in lily.scm and define in this only what is to become the public interface of the package mechanism. All internal code would be loaded from there with (use-modules), or am I missing something here? Where would I store Scheme modules which are *not* to be loaded through ly:load in lily.scm but through (use-modules)? We *do* have a number of such modules in the scm directory. (git grep "(scm " indicates this for the following modules: * accreg * clip-region * coverage * display-lily * editor * framework-eps * framework-null * framework-ps * framework-scm * framework-svg * graphviz * guile-debugger * lily * ly-syntax-constructors * memory-trace * output-ps * output-socket * output-svg * page * paper-system * ps-to-png * safe-utility-defs * song * song-util * to-xml So I could simply store my guile modules there too, but wouldn't it make sense to move them to a new directory to disentangle ly:load from use-modules files? That would seem in line with moving the .ly files like bagpipe.ly which are not loaded automatically but only upon request to packages. Urs
Re: GUILE 2.2 progress
On Sat, Jan 25, 2020 at 1:47 PM Han-Wen Nienhuys wrote: > 7.2 seconds end-to-end includes 1.7s of GC, and 2.0s of reading/compiling SCM. > > On guile 1.8, with GS disabled, the end to end runtime is > > 3.568s including 0.33s for GC and 0.32s for reading the SCM. > > These timings are internally consistent: if we can do byte-compilation > on the .scm files (which would make the SCM reading instantaneous), > and get the GC overhead down, we'd be at the same performance level of > GUILE 1.8. it looks like this should be fixable. The problem is that GC doesn't know how to scale the heap. When we are compiling things, we allocate a lot of new data, but do not generate much garbage. This makes GC waste a lot of time in trying to marking objects, without reclaiming any garbage. You can observe the difference by setting eg. GC_INITIAL_HEAP_SIZE=200M -- Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen
