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

[karlinhigh]

I am trying to understand the origins, motivations, and goals of this
effort to adopt a Code of Conduct. Could its proponents read and comment
on the following blog posts, identifying points of agreement and
disagreement?

https://www.wingolog.org/archives/2017/09/04/the-hardest-thing-about-hiring-is-avoiding-the-fash

https://drewdevault.com/2020/01/17/Effective-project-governance.html

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

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

[nine . fierce . ballads]

On 2020/02/04 23:40:03, dak wrote:
> Personally, the best of two worlds would be if a ping-pong between our
current
> Astyle and clang-format would end up stable after one application of
each.  That
> way, putting the code base through both Astyle and clang-format
semi-regularly
> would put it into a form where users of either formatting tool could
contribute
> their changes without affecting already formatted code in the same
file.

+1.  I actually tried that earlier today, and there were lots of
differences.  I was using astyle 3.1 because that's what I had, so I'm
not sure it represents the ideal, but I'm confident that this
clang-format file needs some work regardless.  It's a good start,
though.

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

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

[dak]

On 2020/02/04 22:18:23, hanwenn wrote:
> On 2020/02/04 20:35:40, Dan Eble wrote:
> > I'm running some of my patches through clang-format as I prepare to
push them.
> > 
> > This is an example of a kind of change it wants to make:
> > 
> > -  const array key {column_rank, dir};
> > +  const array key{column_rank, dir};
> > 
> > Note the space after key.  The setting that controls this is
> > SpaceBeforeCpp11BracedList.  Am I correct that we should use a space
here for
> > consistency with historical style?
> > 
> > More examples of the effect of the option are in the docs:
> >
https://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormatStyleOptions.html
> 
> historically, we didn't have C++11, so this wasn't an issue. I'm fine
with
> whatever you pick.
> 
> With the space seems more coherent with the s p a c e y  GNU style.

Personally, the best of two worlds would be if a ping-pong between our
current Astyle and clang-format would end up stable after one
application of each.  That way, putting the code base through both
Astyle and clang-format semi-regularly would put it into a form where
users of either formatting tool could contribute their changes without
affecting already formatted code in the same file.

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

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

[dak]

What problem are we trying to solve here?

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

Re: development process

[David Kastrup]

Han-Wen Nienhuys  writes:

> My experiences with the current Lilypond development process.
>
> For context, I have a busy daytime job. I work 80% so I can set aside
> a couple of hours of concentrated hacking time on Friday. When I am in
> my element, I can easily churn out a dozen commits in a day. Those
> often touch the same files, because a fix often needs a preparatory
> cleanup (“dependent changes”).
>
> My annoyances so far are especially with the review/countdown process :
>
>
>-
>
>Rietveld + git-cl doesn’t support dependent changes. So to make do, I
>explode my series of changes in individual changes to be reviewed (I
>currently have 34 branches each with a different commit so git-cl can match
>up branch names and reviews). For dependent changes, I have to shepherd the
>base change through review, wait for it to be submitted, and then rebase
>the next change in the series on origin/master.

Changes belonging to the same topic should be committed to the same
Rietveld review and Sourceforge issue.  One can commit them in sequence
to Rietveld when it is desirable to view them independently.  This does
not allow to view fixes resulting from discussion in the context of the
ultimately resulting commit chain (which will usually be fixed
per-commit with git rebase -i).

For a sequence of commits belonging to one logical change, this is the
somewhat defective way of doing stuff.  It's not as bad as you happened
to use it, but it definitely is a tool that grew on Subversion and added
Git as an afterthought.

Where commits do not belong to the same logical issue but are still
interdependent, they cannot be logically disentangled even using a
Git-specific tool like Gerrit.

>Because the review happens on the split-out patches, I now switch back
>and forth between 34 different versions of LilyPond. Every time I address a
>review comment, I go through a lengthy recompile.

Recompiles tend to be very fast unless you "make clean" in between or
check out, in the same work tree, vastly differing branches, like
switching between stable and unstable branches.

Or bisecting across a version change.  It's annoying how much just
depends on the VERSION file, but not actually something that the review
tool will help with.

>The large number of changes clogs up my git branch view.  -
>
>The countdown introduces an extra delay of 2 days in this already
>cumbersome process.
>-
>
>The review process leaves changes in an unclear state. If Werner says
>LGTM, but then Dan and David have complaints, do I have to address the
>comments, or is change already OK to go in?

The change is ok to go in when it has been approved for pushing.  I find
the idea of ignoring instead of addressing complaints weird.

>We track the status of each review in a different system (the bug
>tracker), but the two aren’t linked in an obvious way: I can’t navigate
>from the review to the tracker, for example. Some things (eg. LGTM) are to
>be done in the review tool, but some other things should be in the
>bugtracker.

We don't need to rehash that the current system sucks.  We had to amend
our process after relying on a proprietary tool, namely Google Code,
ended up hanging us out to dry and we had to replace the parts removed.
Our available knowledge and volunteer power ended up with the current
setup which was not intended as a keeper.  It kept the project working,
but I doubt many people will be sad to see it replaced.

>Rietveld and my local commits are not linked. If I change my commits, I
>update my commit message. I have to copy those changes out to Rietveld by
>hand, and it took me quite a while to find the right button (“edit issue”,
>somewhere in the corner).

Then you have set it up incompletely or use it wrong.

git cl upload

will copy the current change set to Rietveld.  I am impressed at the
rate of change you manage to churn out without relying on the commands
we use for that purpose, but this certainly can be done less painfully.


> Some of my complaints come from having to manage a plethora of changes, but
> I suspect the process will trip new contributors up too, to note:
>
>
>-
>
>Seeing your code submitted to a project is what makes coders tick. It is
>the Dopamine hit Facebook exploits so well, and so should we. The key to
>exploiting it is instant gratification, so we should get rid of artificial
>delays
>-
>
>We use “we’ll push if there are no complaints” for contributions. I
>think this is harmful to contributors, because it doesn’t give contributors
>a clear feedback mechanism if they should continue down a path.

They get feedback when the code is getting reviewed.  If code does not
get reviewed, having their changes dropped on the floor is not going to
increase their enthusiasm.

And just above you wanted to know when you are free to ignore feedback.

>It is harmful to the project, 

Re: development process

[Janek Warchoł]

I've just went through the patch upload process (for proposed Code of
Conduct https://codereview.appspot.com/575620043/) and I agree that it
should be changed. It was obscure and confusing even for me.

cheers,
Janek

wt., 4 lut 2020 o 22:57 Han-Wen Nienhuys  napisał(a):

> As promised in several code reviews, here my take on the current
> development process. I wrote it as a google doc first, so you can also go
> here
>
> https://docs.google.com/document/d/1BSffzjiQKMTTmr988ezMbsJyfwT9S-rxGRbYSBTv3PM/edit
> for
> inline comments.
>
>
> Context:
>
>-
>
>https://codereview.appspot.com/561390043/#msg32
>-
>
>
>
> https://docs.google.com/document/d/1zYwygWEKJt21rl-bKL41IpjFmiBjlfob6eS63jd-62I/edit#
>
>
> My experiences with the current Lilypond development process.
>
> For context, I have a busy daytime job. I work 80% so I can set aside a
> couple of hours of concentrated hacking time on Friday. When I am in my
> element, I can easily churn out a dozen commits in a day. Those often touch
> the same files, because a fix often needs a preparatory cleanup (“dependent
> changes”).
>
> My annoyances so far are especially with the review/countdown process :
>
>
>-
>
>Rietveld + git-cl doesn’t support dependent changes. So to make do, I
>explode my series of changes in individual changes to be reviewed (I
>currently have 34 branches each with a different commit so git-cl can
> match
>up branch names and reviews). For dependent changes, I have to shepherd
> the
>base change through review, wait for it to be submitted, and then rebase
>the next change in the series on origin/master.
>-
>
>Because the review happens on the split-out patches, I now switch back
>and forth between 34 different versions of LilyPond. Every time I
> address a
>review comment, I go through a lengthy recompile. The large number of
>changes clogs up my git branch view.
>-
>
>The countdown introduces an extra delay of 2 days in this already
>cumbersome process.
>-
>
>The review process leaves changes in an unclear state. If Werner says
>LGTM, but then Dan and David have complaints, do I have to address the
>comments, or is change already OK to go in?
>-
>
>We track the status of each review in a different system (the bug
>tracker), but the two aren’t linked in an obvious way: I can’t navigate
>from the review to the tracker, for example. Some things (eg. LGTM) are
> to
>be done in the review tool, but some other things should be in the
>bugtracker.
>-
>
>Using the bug tracker to track reviews is new to me. It is common for a
>bug to need multiple changes to be fixed. It also adds another hurdle to
>new developers (setting a sourceforge account and getting that added to
> the
>project).
>-
>
>I have to keep track of my own dashboard: once changes are pushed, I
>have to look them up in Rietveld to click the ‘close’ button.
>-
>
>Rietveld and my local commits are not linked. If I change my commits, I
>update my commit message. I have to copy those changes out to Rietveld
> by
>hand, and it took me quite a while to find the right button (“edit
> issue”,
>somewhere in the corner).
>
>
> Some of my complaints come from having to manage a plethora of changes, but
> I suspect the process will trip new contributors up too, to note:
>
>
>-
>
>Seeing your code submitted to a project is what makes coders tick. It is
>the Dopamine hit Facebook exploits so well, and so should we. The key to
>exploiting it is instant gratification, so we should get rid of
> artificial
>delays
>-
>
>We use “we’ll push if there are no complaints” for contributions. I
>think this is harmful to contributors, because it doesn’t give
> contributors
>a clear feedback mechanism if they should continue down a path. It is
>harmful to the project, because we can end up adopting code that we
> don’t
>understand.
>-
>
>The whole constellation of tools and processes (bug tracker for managing
>patch review, patch testing that seems to involve humans, Rietveld for
>patches, countdown, then push to staging) is odd and different from
>everything else. For contributing, you need to be very passionate about
>music typography, because otherwise, you won’t have the energy to
> invest in
>how the process works.
>
>
> David uses a patch 
> he
> made to GUILE as an example that code can be stuck in a limbo. I disagree
> with this assessment. To me it shows the GUILE community considering and
> then rejecting a patch (comment #26 and #40). I imagine that David was not
> happy about this particular decision, but I see a process working as
> expected. If anything, it took too long and was not explicit enough in
> rejecting the patch. Also, in cases like these, one would normally build
> consensus about the plan before 

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

[janek . lilypond]

Reviewers: pkx166h,

Message:
It seems I cannot create issues on sourceforge despite having an
account...
> Please enter a valid tracker issue number
> (or enter nothing to create a new issue): 
> Error code 403
> Failed URL was
https://sourceforge.net/rest/p/testlilyissues/issues//new

James, can you help?

Description:
>From Mike Solomon, originally discussed at
https://github.com/lilypond/lilypond/pull/8:

> Adds the Contributor Convent Code of Conduct to LilyPond. If we
implemented a
> code of conduct, it would only make sense if we had an enforcement
committee.
> In most organizations, enforcement committee members are exemplary of
the code
> itself.
> 
> The list of adopters is quite robust [1], and I think it has been used
with a lot
> of success. I really hope we can adopt it.
> 
> I would vote for @jan-warchol, @lemzwerg and @hanwen to be the members
of the
> first code of conduct committee.

All suggested members of the committee agree to accept this duty. So,
now the
question is for the whole LilyPond community: do you agree to having
this Code
of Conduct and do you accept the nominees to form the committee?

[1] it includes linux kernel and git project itself.

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

Affected files (+131, -0 lines):
  A CODE_OF_CONDUCT.md

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

[Han-Wen Nienhuys]

On Tue, Feb 4, 2020 at 7:50 PM  wrote:

> On 2020/02/02 22:33:50, hanwenn wrote:
> > For testing, use
> >
> > https://github.com/hanwen/lilypond/tree/guile22-experiment
>
> So I ran this with the large example and I see
> GC Warning: Failed to expand heap by 30635458560 bytes
> a few times (that's 30 GB, my laptop only has 8 GB!!) and finally
> warning: g_spawn_sync failed (0): gs: Failed to fork (Cannot allocate
> memory)
> Maybe exponential growth is not the best choice here? At least my system
> can't handle this score anymore.
>
> Also most of the 'speedup' by this patch seems to come from segments
> _after_ music interpretation (only 10% improvement in music
> interpretation, but around 2x for the total time). I think the later
> parts just benefit from the enormous heap because GC doesn't need to run
> at all? But that's not really the idea of this patch, is it?
>
>
In part, it actually is. If you have a lot of RAM, you should use it
instead of trying to waste CPU cycles recycling it.

do you have a pointer to the score for testing?

Can you do some timings with different values for GC_MAXIMUM_HEAP_SIZE , eg.

 GC_MAXIMUM_HEAP_SIZE=100M

Also, it would be interesting to GC_PRINT_STATS=1 and see how much time is
spent in GC, and how much a typical GC runs reclaim across the process.

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

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

[hanwenn]

On 2020/02/04 20:35:40, Dan Eble wrote:
> I'm running some of my patches through clang-format as I prepare to
push them.
> 
> This is an example of a kind of change it wants to make:
> 
> -  const array key {column_rank, dir};
> +  const array key{column_rank, dir};
> 
> Note the space after key.  The setting that controls this is
> SpaceBeforeCpp11BracedList.  Am I correct that we should use a space
here for
> consistency with historical style?
> 
> More examples of the effect of the option are in the docs:
>
https://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormatStyleOptions.html

historically, we didn't have C++11, so this wasn't an issue. I'm fine
with whatever you pick.

With the space seems more coherent with the s p a c e y  GNU style. 

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

Re: PATCHES - Countdown for February 4th

[Han-Wen Nienhuys]

Somehow https://codereview.appspot.com/577410045/ got lost in the process.
OK to push?

On Tue, Feb 4, 2020 at 7:34 AM  wrote:

> Hello,
>
> Here is the current patch countdown list. The next countdown will be on
> February 6th.
>
> A quick synopsis of all patches currently in the review process can be
> found here:
>
> http://philholmes.net/lilypond/allura/
>
>
> ***
>
>
>   Push:
>
> 5719 Tie formatting maintenance - Dan Eble
> https://sourceforge.net/p/testlilyissues/issues/5719
> http://codereview.appspot.com/581560049
>
> 5717 allow overriding of GUILE_AUTO_COMPILE - Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5717
> http://codereview.appspot.com/579270043
>
> 5713 Remove likely dead code from set_property_from_event - Dan Eble
> https://sourceforge.net/p/testlilyissues/issues/5713
> http://codereview.appspot.com/557260047
>
> 5712 Split context_specification syntax constructor - Dan Eble
> https://sourceforge.net/p/testlilyissues/issues/5712
> http://codereview.appspot.com/565560043
>
> 5703 Run scripts/auxiliar/fixcc.py - David Kastrup
> https://sourceforge.net/p/testlilyissues/issues/5703
> http://codereview.appspot.com/549480043
>
> 5700 Add a tentative .clang-format for LilyPond. - Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5700
> http://codereview.appspot.com/561340043
>
> 5682 Only print out open type font substitution if there was a change -
> Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5682
> http://codereview.appspot.com/577390043
>
> 5674 document and test slur score debugging - Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5674
> http://codereview.appspot.com/555160043
>
> 5362 Generalize context searches - Dan Eble
> https://sourceforge.net/p/testlilyissues/issues/5362
> http://codereview.appspot.com/579250043
>
> 2173 Deal with file names not encoded in UTF-8 - Masamichi Hosoda
> https://sourceforge.net/p/testlilyissues/issues/2173
> http://codereview.appspot.com/575600044
>
>
>   Countdown:
>
> 5732 Use unique_ptr in layout code - Dan Eble
> https://sourceforge.net/p/testlilyissues/issues/5732
> http://codereview.appspot.com/573500043
>
> 5731 Remove outdated comment in scm-hash.hh - David Kastrup
> https://sourceforge.net/p/testlilyissues/issues/5731
> http://codereview.appspot.com/547570043
>
> 5730 Cast to Real in C++ style throughout - Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5730
> http://codereview.appspot.com/547560044
>
> 5729 Fix SyntaxWarning's - Jonas Hahnfeld
> https://sourceforge.net/p/testlilyissues/issues/5729
> http://codereview.appspot.com/559440043
>
> 5728 GUILE v2: set postscript output port to Latin1 - Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5728
> http://codereview.appspot.com/563400065
>
> 5727 Make all int -> Real casts explicit - Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5727
> http://codereview.appspot.com/563460043
>
> 5726 Remove check for rational bugfix. - Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5726
> http://codereview.appspot.com/555230043
>
> 5724 Some cleanups for Python code - Jonas Hahnfeld
> https://sourceforge.net/p/testlilyissues/issues/5724
> http://codereview.appspot.com/551430044
>
> 5723 Pedal_type_info maintenance - Dan Eble
> https://sourceforge.net/p/testlilyissues/issues/5723
> http://codereview.appspot.com/557270049
>
> 5722 lilypond-book: Remove custom package loading - Jonas Hahnfeld
> https://sourceforge.net/p/testlilyissues/issues/5722
> http://codereview.appspot.com/553490051
>
> 5721 lilypond-book: Rewrite processing of snippets - Jonas Hahnfeld
> https://sourceforge.net/p/testlilyissues/issues/5721
> http://codereview.appspot.com/555220043
>
> 5720 Add enabling extension definitions for `-std=c++11` option -
> Masamichi Hosoda
> https://sourceforge.net/p/testlilyissues/issues/5720
> http://codereview.appspot.com/579270051
>
> 5718 Grow heap aggressively during music interpretation - Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5718
> http://codereview.appspot.com/561390043
>
> 5716 Make Pitch::to_string() more robust - Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5716
> http://codereview.appspot.com/581580043
>
> 5715 Use define-syntax for define-session[-public] - Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5715
> http://codereview.appspot.com/553480044
>
> 5714 Document 2 functions in markup-macros.scm - Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5714
> http://codereview.appspot.com/581570043
>
> 5709 Use a pointer for the output parameter of Lily_lexer::scan_word -
> Han-Wen Nienhuys
> https://sourceforge.net/p/testlilyissues/issues/5709
> http://codereview.appspot.com/577440044
>
> 5706 Doc: Correct and extend infos about LilyDev setup - xmichael-k
> https://sourceforge.net/p/testlilyissues/issues/5706
> 

Rewrite define-session and define-session-public macros (issue 549510043 by d...@gnu.org)

[hanwenn]

LGTM



https://codereview.appspot.com/549510043/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[hanwenn]

On 2020/02/04 21:15:44, dak wrote:
> NLGTM
> 
> This patch just punts on using an internal function, and that could be
equally
> well be done without involving syntax-case.
> 
> An alternative proposal that just relies on a single external symbol
in order to
> achieve the original design is given as
> 
> Tracker issue: 5735
(https://sourceforge.net/p/testlilyissues/issues/5735/)
> Rietveld issue: 549510043 (https://codereview.appspot.com/549510043)
> Issue description:
>   Rewrite define-session and define-session-public macros  The byte
>   compiler of Guile-2.x is not able to compile anonymous
>   functions/closures into constants.  Rewriting the macros not to rely
>   on such constants by moving the local functions into their own
>   function definitions is the easiest expedient.
> 
> https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm
> File scm/lily.scm (right):
> 
>
https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode135
> scm/lily.scm:135: (acons (quote name) (make-session-variable (quote
name) value)
> lilypond-exports))
> This is actually a purely textual replacement rather than anything
more complex
> or hygienic.  As such it does nothing that cannot equally well be
achieved using
> defmacro: the problematic issue with the byte compiler was the
internal function
> that was used for _avoiding_ a wholesale textual replacement.
> 
> So drawing in a module that is known to be buggy and unmaintained in
Guile-1.8
> does not even serve a purpose here.

closing in favor of https://codereview.appspot.com/549510043/ - you have
my permission to skip countdown :-)

https://codereview.appspot.com/553480044/

development process

[Han-Wen Nienhuys]

As promised in several code reviews, here my take on the current
development process. I wrote it as a google doc first, so you can also go
here
https://docs.google.com/document/d/1BSffzjiQKMTTmr988ezMbsJyfwT9S-rxGRbYSBTv3PM/edit
for
inline comments.


Context:

   -

   https://codereview.appspot.com/561390043/#msg32
   -


   
https://docs.google.com/document/d/1zYwygWEKJt21rl-bKL41IpjFmiBjlfob6eS63jd-62I/edit#


My experiences with the current Lilypond development process.

For context, I have a busy daytime job. I work 80% so I can set aside a
couple of hours of concentrated hacking time on Friday. When I am in my
element, I can easily churn out a dozen commits in a day. Those often touch
the same files, because a fix often needs a preparatory cleanup (“dependent
changes”).

My annoyances so far are especially with the review/countdown process :


   -

   Rietveld + git-cl doesn’t support dependent changes. So to make do, I
   explode my series of changes in individual changes to be reviewed (I
   currently have 34 branches each with a different commit so git-cl can match
   up branch names and reviews). For dependent changes, I have to shepherd the
   base change through review, wait for it to be submitted, and then rebase
   the next change in the series on origin/master.
   -

   Because the review happens on the split-out patches, I now switch back
   and forth between 34 different versions of LilyPond. Every time I address a
   review comment, I go through a lengthy recompile. The large number of
   changes clogs up my git branch view.
   -

   The countdown introduces an extra delay of 2 days in this already
   cumbersome process.
   -

   The review process leaves changes in an unclear state. If Werner says
   LGTM, but then Dan and David have complaints, do I have to address the
   comments, or is change already OK to go in?
   -

   We track the status of each review in a different system (the bug
   tracker), but the two aren’t linked in an obvious way: I can’t navigate
   from the review to the tracker, for example. Some things (eg. LGTM) are to
   be done in the review tool, but some other things should be in the
   bugtracker.
   -

   Using the bug tracker to track reviews is new to me. It is common for a
   bug to need multiple changes to be fixed. It also adds another hurdle to
   new developers (setting a sourceforge account and getting that added to the
   project).
   -

   I have to keep track of my own dashboard: once changes are pushed, I
   have to look them up in Rietveld to click the ‘close’ button.
   -

   Rietveld and my local commits are not linked. If I change my commits, I
   update my commit message. I have to copy those changes out to Rietveld by
   hand, and it took me quite a while to find the right button (“edit issue”,
   somewhere in the corner).


Some of my complaints come from having to manage a plethora of changes, but
I suspect the process will trip new contributors up too, to note:


   -

   Seeing your code submitted to a project is what makes coders tick. It is
   the Dopamine hit Facebook exploits so well, and so should we. The key to
   exploiting it is instant gratification, so we should get rid of artificial
   delays
   -

   We use “we’ll push if there are no complaints” for contributions. I
   think this is harmful to contributors, because it doesn’t give contributors
   a clear feedback mechanism if they should continue down a path. It is
   harmful to the project, because we can end up adopting code that we don’t
   understand.
   -

   The whole constellation of tools and processes (bug tracker for managing
   patch review, patch testing that seems to involve humans, Rietveld for
   patches, countdown, then push to staging) is odd and different from
   everything else. For contributing, you need to be very passionate about
   music typography, because otherwise, you won’t have the energy to invest in
   how the process works.


David uses a patch  he
made to GUILE as an example that code can be stuck in a limbo. I disagree
with this assessment. To me it shows the GUILE community considering and
then rejecting a patch (comment #26 and #40). I imagine that David was not
happy about this particular decision, but I see a process working as
expected. If anything, it took too long and was not explicit enough in
rejecting the patch. Also, in cases like these, one would normally build
consensus about the plan before going off and writing a patch.

David suggests that I like getting patches in by fiat/countdown, but I
disagree. If you look at the past 2 weeks, you can see that I have actually
tried to address all code review comments so far, and again, it is more
important for the project to have explicit consensus than that individual
contributors that go off in possibly conflicting directions.

Here is what a development process should look to me


   -

   Uncontroversial changes can be submitted 

Re: Janek is coming back to LilyPond! :-)

[Janek Warchoł]

wt., 21 sty 2020 o 19:33 Kieren MacMillan 
napisał(a):

> 1. Wonderful to have you back!
> 2. There’s this lyrics thing maybe you could take a look at…?  ;)


:-)
Hopefully it can be one of the next things I work on. But contributing
process must come first.
J

Re: Janek is coming back to LilyPond! :-)

[Janek Warchoł]

Hello (again),

I guess an explanation of the silence on my side is in place...

For some time now I'm struggling with some personal problems that make me,
well, occasionally "unusable". Unfortunately one such situation occurred
after I wrote, and I managed to pull myself together only this week. I
apologize for lack of follow-up, I should have notified you.

Things should be better at least for some time (it's hard to predict when
something similar will happen to me again). I am sorry that this makes me
not very reliable... I will try to counter this by slicing work into
chunks, to limit the amount of stuff "in progress".

my best wishes to you,
Janek

pon., 20 sty 2020 o 17:34 Janek Warchoł 
napisał(a):

> Hi everyone,
>
> for those who don't know me: I was an active member of the community
> several years ago, contributed patches, co-authored LilyPond blog, and 
> together
> with Urs Liska
>  typeset an 
> award-winning
> publication
> 
>  with
> LilyPond. Since then I've pursued a career in software development.
>
> I just came back from music engraving conference organized by Werner and
> Urs (kudos to them!!), and - inspired by the people present there (most
> importantly Han-Wen, Mike Solomon and Kieren MacMillan) - I decided to get
> involved in the project again :-) I should be able to spend about 5-10
> hours a week on LilyPond.
>
> One of the topics discussed in Salzburg was making it easier to contribute
> to LilyPond. I have proposed to lead that effort, and the people present
> agreed to it. I would like to ask the full developer community for your
> trust*,* too. I will share my plans in a separate thread.
>
> cheers,
> Janek
>

Re: [testlilyissues:issues] #5719 Tie formatting maintenance

[Dan Eble]

> On Feb 4, 2020, at 16:14, Dan Eble  wrote:
> 
> Begin forwarded message:
>> 
>> Subject: [testlilyissues:issues] #5719 Tie formatting maintenance
> ...
>> commit 937e413c0f399b2ec44785b7ca29d61cf7b24cff
>> Author: Dan Eble nine.fierce.ball...@gmail.com
>> Date:   Fri Jan 31 13:48:35 2020 -0500
> 
> The changes for Issue 5719 remove a header file.  If you want to avoid 
> running make clean to reset all dependencies, try removing just these:
> 
>out/lily/tie*
>out/lily/semi-tie*

Oops: those should be lily/out/…
— 
Dan

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[dak]

NLGTM

This patch just punts on using an internal function, and that could be
equally well be done without involving syntax-case.

An alternative proposal that just relies on a single external symbol in
order to achieve the original design is given as

Tracker issue: 5735
(https://sourceforge.net/p/testlilyissues/issues/5735/)
Rietveld issue: 549510043 (https://codereview.appspot.com/549510043)
Issue description:
  Rewrite define-session and define-session-public macros  The byte
  compiler of Guile-2.x is not able to compile anonymous
  functions/closures into constants.  Rewriting the macros not to rely
  on such constants by moving the local functions into their own
  function definitions is the easiest expedient.



https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm
File scm/lily.scm (right):

https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode135
scm/lily.scm:135: (acons (quote name) (make-session-variable (quote
name) value) lilypond-exports))
This is actually a purely textual replacement rather than anything more
complex or hygienic.  As such it does nothing that cannot equally well
be achieved using defmacro: the problematic issue with the byte compiler
was the internal function that was used for _avoiding_ a wholesale
textual replacement.

So drawing in a module that is known to be buggy and unmaintained in
Guile-1.8 does not even serve a purpose here.

https://codereview.appspot.com/553480044/

Fwd: [testlilyissues:issues] #5719 Tie formatting maintenance

[Dan Eble]

Begin forwarded message:
> 
> Subject: [testlilyissues:issues] #5719 Tie formatting maintenance
...
> commit 937e413c0f399b2ec44785b7ca29d61cf7b24cff
> Author: Dan Eble nine.fierce.ball...@gmail.com
> Date:   Fri Jan 31 13:48:35 2020 -0500

The changes for Issue 5719 remove a header file.  If you want to avoid running 
make clean to reset all dependencies, try removing just these:

out/lily/tie*
out/lily/semi-tie*

I think that will be sufficient, but if you hit errors, remove the 
corresponding files.
— 
Dan

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

[nine . fierce . ballads]

I'm running some of my patches through clang-format as I prepare to push
them.

This is an example of a kind of change it wants to make:

-  const array key {column_rank, dir};
+  const array key{column_rank, dir};

Note the space after key.  The setting that controls this is
SpaceBeforeCpp11BracedList.  Am I correct that we should use a space
here for consistency with historical style?

More examples of the effect of the option are in the docs:
https://releases.llvm.org/9.0.0/tools/clang/docs/ClangFormatStyleOptions.html

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

Re: Fix convert-ly with Python 3 (issue 555260044 by jonas.hahnf...@gmail.com)

[jonas . hahnfeld]

https://codereview.appspot.com/555260044/diff/567150044/scripts/convert-ly.py
File scripts/convert-ly.py (right):

https://codereview.appspot.com/555260044/diff/567150044/scripts/convert-ly.py#newcode361
scripts/convert-ly.py:361: f = f
On 2020/02/04 19:45:33, dak wrote:
> That line looks spurious.  Any reason for keeping it in?

Hm, no :D

https://codereview.appspot.com/555260044/

Re: Fix convert-ly with Python 3 (issue 555260044 by jonas.hahnf...@gmail.com)

[dak]

https://codereview.appspot.com/555260044/diff/567150044/scripts/convert-ly.py
File scripts/convert-ly.py (right):

https://codereview.appspot.com/555260044/diff/567150044/scripts/convert-ly.py#newcode361
scripts/convert-ly.py:361: f = f
That line looks spurious.  Any reason for keeping it in?

https://codereview.appspot.com/555260044/

Re: Fix convert-ly with Python 3 (issue 555260044 by jonas.hahnf...@gmail.com)

[jonas . hahnfeld]

Reviewers: dak,

Message:
On 2020/02/04 19:32:10, dak wrote:
> Foreseeable consequences for Python 2.7?

None, because the minimum version is Python 3.5

Description:
Fix convert-ly with Python 3

The error was "'str' object has no attribute 'decode'", so it already
is decoded. The encoding seems to also be correct, as filenames with
Unicode characters work just fine.
It's worrying that none of the usual testing commands seem to invoke
the script with a real input file, only the output of --help is used.

---
If possible I'd like to push this as soon as possible. The normal
process
fails on convert-ly anyhow...

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

Affected files (+1, -1 lines):
  M scripts/convert-ly.py


Index: scripts/convert-ly.py
diff --git a/scripts/convert-ly.py b/scripts/convert-ly.py
index 
ae2ea3161030ae59ca5656675490af0b8cb18301..e4f9f4045ba68b1c3d9b211a26c2ac08a7145da4
 100644
--- a/scripts/convert-ly.py
+++ b/scripts/convert-ly.py
@@ -358,7 +358,7 @@ def main ():
 
 errors = 0
 for f in files:
-f = f.decode (sys.stdin.encoding or "utf-8")
+f = f
 if f == '-':
 f = ''
 elif not os.path.isfile (f):

Fix convert-ly with Python 3 (issue 555260044 by jonas.hahnf...@gmail.com)

[dak]

Foreseeable consequences for Python 2.7?

https://codereview.appspot.com/555260044/

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

[jonas . hahnfeld]

On 2020/02/02 22:33:50, hanwenn wrote:
> For testing, use
> 
> https://github.com/hanwen/lilypond/tree/guile22-experiment

So I ran this with the large example and I see
GC Warning: Failed to expand heap by 30635458560 bytes
a few times (that's 30 GB, my laptop only has 8 GB!!) and finally
warning: g_spawn_sync failed (0): gs: Failed to fork (Cannot allocate
memory)
Maybe exponential growth is not the best choice here? At least my system
can't handle this score anymore.

Also most of the 'speedup' by this patch seems to come from segments
_after_ music interpretation (only 10% improvement in music
interpretation, but around 2x for the total time). I think the later
parts just benefit from the enormous heap because GC doesn't need to run
at all? But that's not really the idea of this patch, is it?

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

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[David Kastrup]

"Phil Holmes"  writes:

> - Original Message - From: "David Kastrup" 
>> Wow.  Ok, maybe I'll just apply this patch then (though I'll at
>> least
>> remove the conditioning on Apple here as the problem is rather likely
>> platform independent) and Phil may have another round.
>> -- 
>> David Kastrup
>
>
> Will kick this off again tomorrow.

Thanks!

-- 
David Kastrup

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Phil Holmes]

- Original Message - 
From: "David Kastrup" 


Wow.  Ok, maybe I'll just apply this patch then (though I'll at least
remove the conditioning on Apple here as the problem is rather likely
platform independent) and Phil may have another round.

--
David Kastrup



Will kick this off again tomorrow.

--
Phil Holmes

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[David Kastrup]

David Kastrup  writes:

> Halving the useful range before overflows is a problem, so I'll stick
> with most of the guards.  Though I am skeptical that stuff exceeding I64
> has much of a chance of working well, anyway.

I have pushed a slightly modified version of the patch (removing the
__APPLE__ guard since that problem is likely only dependent on GCC
version) to stable-2.20.  Let's see where this gets us.

Thanks particularly to Masamichi Hosoda for some really impressive bug
hunt and fast proposing of solutions.

-- 
David Kastrup

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Jonas Hahnfeld]

Am Dienstag, den 04.02.2020, 11:08 -0500 schrieb Dan Eble:
> On Feb 4, 2020, at 11:02, Jonas Hahnfeld <
> hah...@hahnjo.de
> > wrote:
> > > That would be my impulse as well.  It is not like this code appears to
> > > have notable drawbacks for the unafflicted platforms.
> > 
> > Except for very funny overflows and negative signs if the value is too
> > large to fit into I64 ;-P
> > 
> > unsigned long long a = 0xC000;
> > signed long long b = a;
> > printf("%d\n", b);
> > -> -1073741824
> 
> That's already an ingredient of this recipe.
> 
>class Rational
>{
>  ...
>  U64 num_, den_;
>  ...
>  I64 numerator () const { return sign_ * num_; }
> 
> Yum!

Oh wow... I withdraw all my objections, it's already broken if we ever
reach that case.

Jonas


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

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Dan Eble]

On Feb 4, 2020, at 11:02, Jonas Hahnfeld  wrote:
>> That would be my impulse as well.  It is not like this code appears to
>> have notable drawbacks for the unafflicted platforms.
> 
> Except for very funny overflows and negative signs if the value is too
> large to fit into I64 ;-P
> 
> unsigned long long a = 0xC000;
> signed long long b = a;
> printf("%d\n", b);
> -> -1073741824

That's already an ingredient of this recipe.

   class Rational
   {
 ...
 U64 num_, den_;
 ...
 I64 numerator () const { return sign_ * num_; }

Yum!
— 
Dan

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[David Kastrup]

Jonas Hahnfeld  writes:

> Am Dienstag, den 04.02.2020, 16:57 +0100 schrieb David Kastrup:
>> Dan Eble <
>> d...@faithful.be
>> > writes:
>> 
>> > On Feb 4, 2020, at 09:44, Masamichi Hosoda <
>> > truer...@trueroad.jp
>> > > wrote:
>> > > +// FIXME: workaround: In GUB, g++ 4.9.4 for darwin-x86,
>> > > +// it seems that static cast from `unsigned long long` to `double`
>> > > +// by x86 SSE2 raises an internal compile error.
>> > > +// However, static cast from `signed long long` to `double`
>> > > +// does not raise the error.
>> > > +// So we use it for a workaround.
>> > > +#if defined (__i386__) && defined (__APPLE__) && \
>> > > +  defined (__SSE2_MATH__) && __GNUC__ < 5
>> > > +{
>> > > +  I64 inum = num_;
>> > > +  I64 iden = den_;
>> > > +  return static_cast (sign_) *
>> > > +static_cast (inum) / static_cast (iden);
>> > > +}
>> > > +#else
>> > > return (double)sign_ * (double)num_ / (double)den_;
>> > > +#endif
>> > 
>> > Is the conditional code really necessary?  Why not boil it down to the
>> > working code and a comment explaining the extra conversion to signed
>> > numbers?
>> 
>> That would be my impulse as well.  It is not like this code appears to
>> have notable drawbacks for the unafflicted platforms.
>
> Except for very funny overflows and negative signs if the value is too
> large to fit into I64 ;-P
>
> unsigned long long a = 0xC000;
> signed long long b = a;
> printf("%d\n", b);
> -> -1073741824

Halving the useful range before overflows is a problem, so I'll stick
with most of the guards.  Though I am skeptical that stuff exceeding I64
has much of a chance of working well, anyway.

-- 
David Kastrup

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Jonas Hahnfeld]

Am Dienstag, den 04.02.2020, 17:03 +0100 schrieb David Kastrup:
> Jonas Hahnfeld <
> hah...@hahnjo.de
> > writes:
> 
> > Am Dienstag, den 04.02.2020, 10:38 -0500 schrieb Dan Eble:
> > > > On Feb 4, 2020, at 10:32, Jonas Hahnfeld <
> > > > hah...@hahnjo.de
> > > > 
> > > > > wrote:
> > > > 
> > > > Am Mittwoch, den 05.02.2020, 00:24 +0900 schrieb Masamichi Hosoda:
> > > > > > > $
> > > > > > > ~gub/gub/target/linux-x86/root/usr/cross/bin/i686-linux-g++
> > > > > > > -c -msse2 -mfpmath=sse -I include -I .. rational.cc
> > > > > > > $
> > > > > > > ~gub/gub/target/freebsd-x86/root/usr/cross/bin/i686-freebsd6-g++
> > > > > > > -c -msse2 -mfpmath=sse -I include -I .. rational.cc
> > > > > > 
> > > > > > So this only affects darwin-x86 which confirms my observation that
> > > > > > linux-x86 still works. AFAICS darwin-x86 never had a fpu_control.h, 
> > > > > > so
> > > > > > I'd propose to drop -msse2 -mfpmath=sse for i686-apple-darwin8.
> > > > > > This unblocks the release and the situation on darwin-x86 remains 
> > > > > > the
> > > > > > same as before, but we have the fix for linux-x86 and mingw.
> > > > > > My fear is that not only rational.cc is affected, but also many 
> > > > > > other
> > > > > > files. Did you test if a full compile works? If no, I'm against 
> > > > > > adding
> > > > > > workarounds for all files that suffer from the compiler error.
> > > > > 
> > > > > 
> > > > > I didn't test full compiler works.
> > > > > However, If I understand correctly,
> > > > > it seems that static cast from `unsigned long long` to `double`
> > > > > is only in rational.cc.
> > > > 
> > > > What makes you think so? I think you're right it's the only place with
> > > > (double) casting, but I see a few with double (...) and some more with
> > > > Real (...).
> > > > In particular, flower/cpu-timer.cc casts variables of type clock_t -
> > > > I've yet to hunt down which type of integer this actually is.
> > > > 
> > > > Jonas
> > > 
> > > And what about size_t to Real?  There's some of that in the queue:
> > > https://codereview.appspot.com/563460043
> > > 
> > 
> > Apparently the build goes through. If I see this correctly, both
> > clock_t and size_t are "unsigned long".
> 
> Wow.  Ok, maybe I'll just apply this patch then (though I'll at least
> remove the conditioning on Apple here as the problem is rather likely
> platform independent) and Phil may have another round.

Do we have a guarantee that we never overflow with this cast?

Jonas


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

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[David Kastrup]

Jonas Hahnfeld  writes:

> Am Dienstag, den 04.02.2020, 10:38 -0500 schrieb Dan Eble:
>> > On Feb 4, 2020, at 10:32, Jonas Hahnfeld <
>> > hah...@hahnjo.de
>> > > wrote:
>> > 
>> > Am Mittwoch, den 05.02.2020, 00:24 +0900 schrieb Masamichi Hosoda:
>> > > > > $
>> > > > > ~gub/gub/target/linux-x86/root/usr/cross/bin/i686-linux-g++
>> > > > > -c -msse2 -mfpmath=sse -I include -I .. rational.cc
>> > > > > $
>> > > > > ~gub/gub/target/freebsd-x86/root/usr/cross/bin/i686-freebsd6-g++
>> > > > > -c -msse2 -mfpmath=sse -I include -I .. rational.cc
>> > > > So this only affects darwin-x86 which confirms my observation that
>> > > > linux-x86 still works. AFAICS darwin-x86 never had a fpu_control.h, so
>> > > > I'd propose to drop -msse2 -mfpmath=sse for i686-apple-darwin8.
>> > > > This unblocks the release and the situation on darwin-x86 remains the
>> > > > same as before, but we have the fix for linux-x86 and mingw.
>> > > > My fear is that not only rational.cc is affected, but also many other
>> > > > files. Did you test if a full compile works? If no, I'm against adding
>> > > > workarounds for all files that suffer from the compiler error.
>> > > 
>> > > 
>> > > I didn't test full compiler works.
>> > > However, If I understand correctly,
>> > > it seems that static cast from `unsigned long long` to `double`
>> > > is only in rational.cc.
>> > 
>> > What makes you think so? I think you're right it's the only place with
>> > (double) casting, but I see a few with double (...) and some more with
>> > Real (...).
>> > In particular, flower/cpu-timer.cc casts variables of type clock_t -
>> > I've yet to hunt down which type of integer this actually is.
>> > 
>> > Jonas
>> 
>> And what about size_t to Real?  There's some of that in the queue:
>> https://codereview.appspot.com/563460043
>
> Apparently the build goes through. If I see this correctly, both
> clock_t and size_t are "unsigned long".

Wow.  Ok, maybe I'll just apply this patch then (though I'll at least
remove the conditioning on Apple here as the problem is rather likely
platform independent) and Phil may have another round.

-- 
David Kastrup

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Jonas Hahnfeld]

Am Dienstag, den 04.02.2020, 16:57 +0100 schrieb David Kastrup:
> Dan Eble <
> d...@faithful.be
> > writes:
> 
> > On Feb 4, 2020, at 09:44, Masamichi Hosoda <
> > truer...@trueroad.jp
> > > wrote:
> > > +// FIXME: workaround: In GUB, g++ 4.9.4 for darwin-x86,
> > > +// it seems that static cast from `unsigned long long` to `double`
> > > +// by x86 SSE2 raises an internal compile error.
> > > +// However, static cast from `signed long long` to `double`
> > > +// does not raise the error.
> > > +// So we use it for a workaround.
> > > +#if defined (__i386__) && defined (__APPLE__) && \
> > > +  defined (__SSE2_MATH__) && __GNUC__ < 5
> > > +{
> > > +  I64 inum = num_;
> > > +  I64 iden = den_;
> > > +  return static_cast (sign_) *
> > > +static_cast (inum) / static_cast (iden);
> > > +}
> > > +#else
> > > return (double)sign_ * (double)num_ / (double)den_;
> > > +#endif
> > 
> > Is the conditional code really necessary?  Why not boil it down to the
> > working code and a comment explaining the extra conversion to signed
> > numbers?
> 
> That would be my impulse as well.  It is not like this code appears to
> have notable drawbacks for the unafflicted platforms.

Except for very funny overflows and negative signs if the value is too
large to fit into I64 ;-P

unsigned long long a = 0xC000;
signed long long b = a;
printf("%d\n", b);
-> -1073741824

Jonas


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

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Jonas Hahnfeld]

Am Dienstag, den 04.02.2020, 10:38 -0500 schrieb Dan Eble:
> > On Feb 4, 2020, at 10:32, Jonas Hahnfeld <
> > hah...@hahnjo.de
> > > wrote:
> > 
> > Am Mittwoch, den 05.02.2020, 00:24 +0900 schrieb Masamichi Hosoda:
> > > > > $ ~gub/gub/target/linux-x86/root/usr/cross/bin/i686-linux-g++ -c 
> > > > > -msse2 -mfpmath=sse -I include -I .. rational.cc
> > > > > $ ~gub/gub/target/freebsd-x86/root/usr/cross/bin/i686-freebsd6-g++ -c 
> > > > > -msse2 -mfpmath=sse -I include -I .. rational.cc
> > > > So this only affects darwin-x86 which confirms my observation that
> > > > linux-x86 still works. AFAICS darwin-x86 never had a fpu_control.h, so
> > > > I'd propose to drop -msse2 -mfpmath=sse for i686-apple-darwin8.
> > > > This unblocks the release and the situation on darwin-x86 remains the
> > > > same as before, but we have the fix for linux-x86 and mingw.
> > > > My fear is that not only rational.cc is affected, but also many other
> > > > files. Did you test if a full compile works? If no, I'm against adding
> > > > workarounds for all files that suffer from the compiler error.
> > > 
> > > 
> > > I didn't test full compiler works.
> > > However, If I understand correctly,
> > > it seems that static cast from `unsigned long long` to `double`
> > > is only in rational.cc.
> > 
> > What makes you think so? I think you're right it's the only place with
> > (double) casting, but I see a few with double (...) and some more with
> > Real (...).
> > In particular, flower/cpu-timer.cc casts variables of type clock_t -
> > I've yet to hunt down which type of integer this actually is.
> > 
> > Jonas
> 
> And what about size_t to Real?  There's some of that in the queue:
> https://codereview.appspot.com/563460043

Apparently the build goes through. If I see this correctly, both
clock_t and size_t are "unsigned long".

Jonas


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

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[David Kastrup]

Dan Eble  writes:

> On Feb 4, 2020, at 09:44, Masamichi Hosoda  wrote:
>> 
>> +// FIXME: workaround: In GUB, g++ 4.9.4 for darwin-x86,
>> +// it seems that static cast from `unsigned long long` to `double`
>> +// by x86 SSE2 raises an internal compile error.
>> +// However, static cast from `signed long long` to `double`
>> +// does not raise the error.
>> +// So we use it for a workaround.
>> +#if defined (__i386__) && defined (__APPLE__) && \
>> +  defined (__SSE2_MATH__) && __GNUC__ < 5
>> +{
>> +  I64 inum = num_;
>> +  I64 iden = den_;
>> +  return static_cast (sign_) *
>> +static_cast (inum) / static_cast (iden);
>> +}
>> +#else
>> return (double)sign_ * (double)num_ / (double)den_;
>> +#endif
>
> Is the conditional code really necessary?  Why not boil it down to the
> working code and a comment explaining the extra conversion to signed
> numbers?

That would be my impulse as well.  It is not like this code appears to
have notable drawbacks for the unafflicted platforms.

-- 
David Kastrup

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Dan Eble]

> On Feb 4, 2020, at 10:32, Jonas Hahnfeld  wrote:
> 
> Am Mittwoch, den 05.02.2020, 00:24 +0900 schrieb Masamichi Hosoda:
 $ ~gub/gub/target/linux-x86/root/usr/cross/bin/i686-linux-g++ -c -msse2 
 -mfpmath=sse -I include -I .. rational.cc
>> 
 $ ~gub/gub/target/freebsd-x86/root/usr/cross/bin/i686-freebsd6-g++ -c 
 -msse2 -mfpmath=sse -I include -I .. rational.cc
>> 
>>> 
>> 
>>> So this only affects darwin-x86 which confirms my observation that
>> 
>>> linux-x86 still works. AFAICS darwin-x86 never had a fpu_control.h, so
>> 
>>> I'd propose to drop -msse2 -mfpmath=sse for i686-apple-darwin8.
>> 
>>> This unblocks the release and the situation on darwin-x86 remains the
>> 
>>> same as before, but we have the fix for linux-x86 and mingw.
>> 
>>> 
>> 
>>> My fear is that not only rational.cc is affected, but also many other
>> 
>>> files. Did you test if a full compile works? If no, I'm against adding
>> 
>>> workarounds for all files that suffer from the compiler error.
>> 
>> 
>> I didn't test full compiler works.
>> However, If I understand correctly,
>> it seems that static cast from `unsigned long long` to `double`
>> is only in rational.cc.
> 
> What makes you think so? I think you're right it's the only place with
> (double) casting, but I see a few with double (...) and some more with
> Real (...).
> In particular, flower/cpu-timer.cc casts variables of type clock_t -
> I've yet to hunt down which type of integer this actually is.
> 
> Jonas

And what about size_t to Real?  There's some of that in the queue:
https://codereview.appspot.com/563460043
— 
Dan

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Dan Eble]

On Feb 4, 2020, at 09:44, Masamichi Hosoda  wrote:
> 
> +// FIXME: workaround: In GUB, g++ 4.9.4 for darwin-x86,
> +// it seems that static cast from `unsigned long long` to `double`
> +// by x86 SSE2 raises an internal compile error.
> +// However, static cast from `signed long long` to `double`
> +// does not raise the error.
> +// So we use it for a workaround.
> +#if defined (__i386__) && defined (__APPLE__) && \
> +  defined (__SSE2_MATH__) && __GNUC__ < 5
> +{
> +  I64 inum = num_;
> +  I64 iden = den_;
> +  return static_cast (sign_) *
> +static_cast (inum) / static_cast (iden);
> +}
> +#else
> return (double)sign_ * (double)num_ / (double)den_;
> +#endif

Is the conditional code really necessary?  Why not boil it down to the working 
code and a comment explaining the extra conversion to signed numbers?
— 
Dan

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Jonas Hahnfeld]

Am Mittwoch, den 05.02.2020, 00:24 +0900 schrieb Masamichi Hosoda:
> >> $ ~gub/gub/target/linux-x86/root/usr/cross/bin/i686-linux-g++ -c -msse2 
> >> -mfpmath=sse -I include -I .. rational.cc
> 
> >> $ ~gub/gub/target/freebsd-x86/root/usr/cross/bin/i686-freebsd6-g++ -c 
> >> -msse2 -mfpmath=sse -I include -I .. rational.cc
> 
> > 
> 
> > So this only affects darwin-x86 which confirms my observation that
> 
> > linux-x86 still works. AFAICS darwin-x86 never had a fpu_control.h, so
> 
> > I'd propose to drop -msse2 -mfpmath=sse for i686-apple-darwin8.
> 
> > This unblocks the release and the situation on darwin-x86 remains the
> 
> > same as before, but we have the fix for linux-x86 and mingw.
> 
> > 
> 
> > My fear is that not only rational.cc is affected, but also many other
> 
> > files. Did you test if a full compile works? If no, I'm against adding
> 
> > workarounds for all files that suffer from the compiler error.
> 
> 
> I didn't test full compiler works.
> However, If I understand correctly,
> it seems that static cast from `unsigned long long` to `double`
> is only in rational.cc.

What makes you think so? I think you're right it's the only place with
(double) casting, but I see a few with double (...) and some more with
Real (...).
In particular, flower/cpu-timer.cc casts variables of type clock_t -
I've yet to hunt down which type of integer this actually is.

Jonas


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

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Masamichi Hosoda]

>> $ ~gub/gub/target/linux-x86/root/usr/cross/bin/i686-linux-g++ -c -msse2 
>> -mfpmath=sse -I include -I .. rational.cc
>> $ ~gub/gub/target/freebsd-x86/root/usr/cross/bin/i686-freebsd6-g++ -c -msse2 
>> -mfpmath=sse -I include -I .. rational.cc
> 
> So this only affects darwin-x86 which confirms my observation that
> linux-x86 still works. AFAICS darwin-x86 never had a fpu_control.h, so
> I'd propose to drop -msse2 -mfpmath=sse for i686-apple-darwin8.
> This unblocks the release and the situation on darwin-x86 remains the
> same as before, but we have the fix for linux-x86 and mingw.
> 
> My fear is that not only rational.cc is affected, but also many other
> files. Did you test if a full compile works? If no, I'm against adding
> workarounds for all files that suffer from the compiler error.

I didn't test full compiler works.
However, If I understand correctly,
it seems that static cast from `unsigned long long` to `double`
is only in rational.cc.

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Phil Holmes]

- Original Message - 
From: "David Kastrup" 

To: "Masamichi Hosoda" 
Cc: ; 
Sent: Tuesday, February 04, 2020 2:56 PM
Subject: Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW 
libraries (issue 577450043 by thomasmorle...@gmail.com)




Wouldn't the same problem occur on Windows?  We have 32bit executables
there as well.  Or is the compiler version we use there not afflicted?

--
David Kastrup


I'd suggest applying Hosoda-san's patch to stable/2.20 and I'll run Gub 
again.  If it runs OK, we're good.  If not, we can revert and rethink?


--
Phil Holmes 

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Masamichi Hosoda]

> We currently have the problem that the compiler used in GUB for
> compiling 32bit binaries gets an internal compiler fault for those
> options.
> 
> We'll need to figure out whether a newer compiler does the trick, and if
> it does, update GUB.  Or find a different way of proceeding.
> 
> I'll see whether I can convince my current compilers to generate 32bit
> code and see whether those are currently up to using those options.  If
> current compilers don't want them, we'll need to revert to a different
> plan.

 It seems that static cast from `unsigned long long` to `double`
 by x86 SSE2 raises the internal compile error.
 However, static cast from `signed long long` to `double`
 does not raise the errir.
 I think it can be a workaround for rational.cc.

 i.e.
 First, static cast from `unsigned long long` to `signed long long`
 Then, static cast from `singed long long` to `double`
>>> 
>>> Oh wow.  I would never have thought one could identify something as
>>> specific.  I think I have convinced my system to build a 32bit Guile,
>>> but have problems convincing LilyPond to do the same.  So I cannot help
>>> with debugging this situation yet or even finding out whether it
>>> persists into newer compiler versions.
>>
>> I've attached the workaround patch for stable/2.20 branch.
>> The results of my experiment is here.
> 
> 
>>>From 2a3816e49067e026c7180dc6a3b2d5614d9c20d6 Mon Sep 17 00:00:00 2001
>> From: Masamichi Hosoda 
>> Date: Tue, 4 Feb 2020 23:30:29 +0900
>> Subject: [PATCH] Add workaround for avoiding GUB darwin-x86 error
>>
>> In GUB, g++ 4.9.4 for darwin-x86 (macOS x86),
>> it seems that static cast from  `unsigned long long` to `double`
>> by x86 SSE2 raises an internal compile error.
>> However, static cast from `signed long long` to `double`
>> does not raise the error.
>> So we use it for a workaround.
>>
>> i.e.
>> First, static cast from `unsigned long long` to `signed long long`.
>> Then, static cast from `singed long long` to `double`.
>> ---
>>  flower/rational.cc | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/flower/rational.cc b/flower/rational.cc
>> index 559e1646a0..9435edbb8f 100644
>> --- a/flower/rational.cc
>> +++ b/flower/rational.cc
>> @@ -31,7 +31,23 @@ double
>>  Rational::to_double () const
>>  {
>>if (sign_ == -1 || sign_ == 1 || sign_ == 0)
>> +// FIXME: workaround: In GUB, g++ 4.9.4 for darwin-x86,
>> +// it seems that static cast from `unsigned long long` to `double`
>> +// by x86 SSE2 raises an internal compile error.
>> +// However, static cast from `signed long long` to `double`
>> +// does not raise the error.
>> +// So we use it for a workaround.
>> +#if defined (__i386__) && defined (__APPLE__) && \
>> +  defined (__SSE2_MATH__) && __GNUC__ < 5
> 
> Wouldn't the same problem occur on Windows?  We have 32bit executables
> there as well.  Or is the compiler version we use there not afflicted?

Sorry, I forgot MinGW.
The internal compiler error seems to raise only in darwin-x86.
I don't have newer g++ for darwin-x86, so I'm not sure
if it was fixed in the newer g++.

without the patch:

```
$ ~gub/gub/target/darwin-x86/root/usr/cross/bin/i686-apple-darwin8-g++ -c 
-msse2 -mfpmath=sse -I include -I .. rational.cc
rational.cc:36:5: warning: floating constant exceeds range of 'double' 
[-Woverflow]
 return -HUGE_VAL;
 ^
rational.cc:38:5: warning: floating constant exceeds range of 'double' 
[-Woverflow]
 return HUGE_VAL;
 ^
rational.cc: In member function 'double Rational::to_double() const':
rational.cc:43:1: internal compiler error: in gen_reg_rtx, at emit-rtl.c:838
 }
 ^
0x773934 gen_reg_rtx(machine_mode)
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/emit-rtl.c:838
0xc53c53 gen_split_4130(rtx_def*, rtx_def**)

/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/config/i386/sse.md:884
0x7772a7 try_split(rtx_def*, rtx_def*, int)
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/emit-rtl.c:3444
0x8feb71 split_insn
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:2897
0x9034e4 split_all_insns()
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:2987
0x903578 rest_of_handle_split_after_reload
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:3938
0x903578 execute
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:3967
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See  for instructions.
$ ~gub/gub/target/linux-x86/root/usr/cross/bin/i686-linux-g++ -c -msse2 
-mfpmath=sse -I include -I .. rational.cc
$ ~gub/gub/target/freebsd-x86/root/usr/cross/bin/i686-freebsd6-g++ -c -msse2 
-mfpmath=sse -I include -I .. rational.cc
$ ~gub/gub/target/mingw/root/usr/cross/bin/i686-mingw32-g++ -c -msse2 

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[David Kastrup]

Masamichi Hosoda  writes:

 We currently have the problem that the compiler used in GUB for
 compiling 32bit binaries gets an internal compiler fault for those
 options.
 
 We'll need to figure out whether a newer compiler does the trick, and if
 it does, update GUB.  Or find a different way of proceeding.
 
 I'll see whether I can convince my current compilers to generate 32bit
 code and see whether those are currently up to using those options.  If
 current compilers don't want them, we'll need to revert to a different
 plan.
>>>
>>> It seems that static cast from `unsigned long long` to `double`
>>> by x86 SSE2 raises the internal compile error.
>>> However, static cast from `signed long long` to `double`
>>> does not raise the errir.
>>> I think it can be a workaround for rational.cc.
>>>
>>> i.e.
>>> First, static cast from `unsigned long long` to `signed long long`
>>> Then, static cast from `singed long long` to `double`
>> 
>> Oh wow.  I would never have thought one could identify something as
>> specific.  I think I have convinced my system to build a 32bit Guile,
>> but have problems convincing LilyPond to do the same.  So I cannot help
>> with debugging this situation yet or even finding out whether it
>> persists into newer compiler versions.
>
> I've attached the workaround patch for stable/2.20 branch.
> The results of my experiment is here.


>>From 2a3816e49067e026c7180dc6a3b2d5614d9c20d6 Mon Sep 17 00:00:00 2001
> From: Masamichi Hosoda 
> Date: Tue, 4 Feb 2020 23:30:29 +0900
> Subject: [PATCH] Add workaround for avoiding GUB darwin-x86 error
>
> In GUB, g++ 4.9.4 for darwin-x86 (macOS x86),
> it seems that static cast from  `unsigned long long` to `double`
> by x86 SSE2 raises an internal compile error.
> However, static cast from `signed long long` to `double`
> does not raise the error.
> So we use it for a workaround.
>
> i.e.
> First, static cast from `unsigned long long` to `signed long long`.
> Then, static cast from `singed long long` to `double`.
> ---
>  flower/rational.cc | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/flower/rational.cc b/flower/rational.cc
> index 559e1646a0..9435edbb8f 100644
> --- a/flower/rational.cc
> +++ b/flower/rational.cc
> @@ -31,7 +31,23 @@ double
>  Rational::to_double () const
>  {
>if (sign_ == -1 || sign_ == 1 || sign_ == 0)
> +// FIXME: workaround: In GUB, g++ 4.9.4 for darwin-x86,
> +// it seems that static cast from `unsigned long long` to `double`
> +// by x86 SSE2 raises an internal compile error.
> +// However, static cast from `signed long long` to `double`
> +// does not raise the error.
> +// So we use it for a workaround.
> +#if defined (__i386__) && defined (__APPLE__) && \
> +  defined (__SSE2_MATH__) && __GNUC__ < 5

Wouldn't the same problem occur on Windows?  We have 32bit executables
there as well.  Or is the compiler version we use there not afflicted?

-- 
David Kastrup

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Jonas Hahnfeld]

Am Dienstag, den 04.02.2020, 23:44 +0900 schrieb Masamichi Hosoda:
> >>> We currently have the problem that the compiler used in GUB for
> 
> >>> compiling 32bit binaries gets an internal compiler fault for those
> 
> >>> options.
> 
> >>> 
> 
> >>> We'll need to figure out whether a newer compiler does the trick, and if
> 
> >>> it does, update GUB.  Or find a different way of proceeding.
> 
> >>> 
> 
> >>> I'll see whether I can convince my current compilers to generate 32bit
> 
> >>> code and see whether those are currently up to using those options.  If
> 
> >>> current compilers don't want them, we'll need to revert to a different
> 
> >>> plan.
> 
> >>
> 
> >> It seems that static cast from `unsigned long long` to `double`
> 
> >> by x86 SSE2 raises the internal compile error.
> 
> >> However, static cast from `signed long long` to `double`
> 
> >> does not raise the errir.
> 
> >> I think it can be a workaround for rational.cc.
> 
> >>
> 
> >> i.e.
> 
> >> First, static cast from `unsigned long long` to `signed long long`
> 
> >> Then, static cast from `singed long long` to `double`
> 
> > 
> 
> > Oh wow.  I would never have thought one could identify something as
> 
> > specific.  I think I have convinced my system to build a 32bit Guile,
> 
> > but have problems convincing LilyPond to do the same.  So I cannot help
> 
> > with debugging this situation yet or even finding out whether it
> 
> > persists into newer compiler versions.
> 
> 
> I've attached the workaround patch for stable/2.20 branch.
> The results of my experiment is here.
> 
> Current stable/2.20 bcad34e31d7866eb126e73abc89eeeb01faf081f without the 
> patch:
> 
> ```
> $ ~gub/gub/target/darwin-x86/root/usr/cross/bin/i686-apple-darwin8-g++ -c 
> -msse2 -mfpmath=sse -I include -I .. rational.cc
> rational.cc: In member function 'double Rational::to_double() const':
> rational.cc:48:1: internal compiler error: in gen_reg_rtx, at emit-rtl.c:838
>  }
>  ^
> 0x773934 gen_reg_rtx(machine_mode)
> /home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/emit-rtl.c:838
> 0xc53c53 gen_split_4130(rtx_def*, rtx_def**)
> 
> /home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/config/i386/sse.md:884
> 0x7772a7 try_split(rtx_def*, rtx_def*, int)
> 
> /home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/emit-rtl.c:3444
> 0x8feb71 split_insn
> /home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:2897
> 0x9034e4 split_all_insns()
> /home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:2987
> 0x903578 rest_of_handle_split_after_reload
> /home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:3938
> 0x903578 execute
> /home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:3967
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See <
> http://gcc.gnu.org/bugs.html
> > for instructions.
> $ ~gub/gub/target/linux-x86/root/usr/cross/bin/i686-linux-g++ -c -msse2 
> -mfpmath=sse -I include -I .. rational.cc
> $ ~gub/gub/target/freebsd-x86/root/usr/cross/bin/i686-freebsd6-g++ -c -msse2 
> -mfpmath=sse -I include -I .. rational.cc

So this only affects darwin-x86 which confirms my observation that
linux-x86 still works. AFAICS darwin-x86 never had a fpu_control.h, so
I'd propose to drop -msse2 -mfpmath=sse for i686-apple-darwin8.
This unblocks the release and the situation on darwin-x86 remains the
same as before, but we have the fix for linux-x86 and mingw.

My fear is that not only rational.cc is affected, but also many other
files. Did you test if a full compile works? If no, I'm against adding
workarounds for all files that suffer from the compiler error.

Regards,
Jonas

> $
> ```
> 
> Current stable/2.20 bcad34e31d7866eb126e73abc89eeeb01faf081f with the patch:
> 
> ```
> $ ~gub/gub/target/darwin-x86/root/usr/cross/bin/i686-apple-darwin8-g++ -c 
> -msse2 -mfpmath=sse -I include -I .. rational.cc
> rational.cc:52:5: warning: floating constant exceeds range of 'double' 
> [-Woverflow]
>  return -HUGE_VAL;
>  ^
> rational.cc:54:5: warning: floating constant exceeds range of 'double' 
> [-Woverflow]
>  return HUGE_VAL;
>  ^
> $ ~gub/gub/target/linux-x86/root/usr/cross/bin/i686-linux-g++ -c -msse2 
> -mfpmath=sse -I include -I .. rational.cc
> $ ~gub/gub/target/freebsd-x86/root/usr/cross/bin/i686-freebsd6-g++ -c -msse2 
> -mfpmath=sse -I include -I .. rational.cc
> $
> ```
> 


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

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Masamichi Hosoda]

>>> We currently have the problem that the compiler used in GUB for
>>> compiling 32bit binaries gets an internal compiler fault for those
>>> options.
>>> 
>>> We'll need to figure out whether a newer compiler does the trick, and if
>>> it does, update GUB.  Or find a different way of proceeding.
>>> 
>>> I'll see whether I can convince my current compilers to generate 32bit
>>> code and see whether those are currently up to using those options.  If
>>> current compilers don't want them, we'll need to revert to a different
>>> plan.
>>
>> It seems that static cast from `unsigned long long` to `double`
>> by x86 SSE2 raises the internal compile error.
>> However, static cast from `signed long long` to `double`
>> does not raise the errir.
>> I think it can be a workaround for rational.cc.
>>
>> i.e.
>> First, static cast from `unsigned long long` to `signed long long`
>> Then, static cast from `singed long long` to `double`
> 
> Oh wow.  I would never have thought one could identify something as
> specific.  I think I have convinced my system to build a 32bit Guile,
> but have problems convincing LilyPond to do the same.  So I cannot help
> with debugging this situation yet or even finding out whether it
> persists into newer compiler versions.

I've attached the workaround patch for stable/2.20 branch.
The results of my experiment is here.

Current stable/2.20 bcad34e31d7866eb126e73abc89eeeb01faf081f without the patch:

```
$ ~gub/gub/target/darwin-x86/root/usr/cross/bin/i686-apple-darwin8-g++ -c 
-msse2 -mfpmath=sse -I include -I .. rational.cc
rational.cc: In member function 'double Rational::to_double() const':
rational.cc:48:1: internal compiler error: in gen_reg_rtx, at emit-rtl.c:838
 }
 ^
0x773934 gen_reg_rtx(machine_mode)
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/emit-rtl.c:838
0xc53c53 gen_split_4130(rtx_def*, rtx_def**)

/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/config/i386/sse.md:884
0x7772a7 try_split(rtx_def*, rtx_def*, int)
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/emit-rtl.c:3444
0x8feb71 split_insn
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:2897
0x9034e4 split_all_insns()
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:2987
0x903578 rest_of_handle_split_after_reload
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:3938
0x903578 execute
/home/gub/gub/target/darwin-x86/src/cross/gcc-4.9.4/gcc/recog.c:3967
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See  for instructions.
$ ~gub/gub/target/linux-x86/root/usr/cross/bin/i686-linux-g++ -c -msse2 
-mfpmath=sse -I include -I .. rational.cc
$ ~gub/gub/target/freebsd-x86/root/usr/cross/bin/i686-freebsd6-g++ -c -msse2 
-mfpmath=sse -I include -I .. rational.cc
$
```

Current stable/2.20 bcad34e31d7866eb126e73abc89eeeb01faf081f with the patch:

```
$ ~gub/gub/target/darwin-x86/root/usr/cross/bin/i686-apple-darwin8-g++ -c 
-msse2 -mfpmath=sse -I include -I .. rational.cc
rational.cc:52:5: warning: floating constant exceeds range of 'double' 
[-Woverflow]
 return -HUGE_VAL;
 ^
rational.cc:54:5: warning: floating constant exceeds range of 'double' 
[-Woverflow]
 return HUGE_VAL;
 ^
$ ~gub/gub/target/linux-x86/root/usr/cross/bin/i686-linux-g++ -c -msse2 
-mfpmath=sse -I include -I .. rational.cc
$ ~gub/gub/target/freebsd-x86/root/usr/cross/bin/i686-freebsd6-g++ -c -msse2 
-mfpmath=sse -I include -I .. rational.cc
$
```
>From 2a3816e49067e026c7180dc6a3b2d5614d9c20d6 Mon Sep 17 00:00:00 2001
From: Masamichi Hosoda 
Date: Tue, 4 Feb 2020 23:30:29 +0900
Subject: [PATCH] Add workaround for avoiding GUB darwin-x86 error

In GUB, g++ 4.9.4 for darwin-x86 (macOS x86),
it seems that static cast from  `unsigned long long` to `double`
by x86 SSE2 raises an internal compile error.
However, static cast from `signed long long` to `double`
does not raise the error.
So we use it for a workaround.

i.e.
First, static cast from `unsigned long long` to `signed long long`.
Then, static cast from `singed long long` to `double`.
---
 flower/rational.cc | 16 
 1 file changed, 16 insertions(+)

diff --git a/flower/rational.cc b/flower/rational.cc
index 559e1646a0..9435edbb8f 100644
--- a/flower/rational.cc
+++ b/flower/rational.cc
@@ -31,7 +31,23 @@ double
 Rational::to_double () const
 {
   if (sign_ == -1 || sign_ == 1 || sign_ == 0)
+// FIXME: workaround: In GUB, g++ 4.9.4 for darwin-x86,
+// it seems that static cast from `unsigned long long` to `double`
+// by x86 SSE2 raises an internal compile error.
+// However, static cast from `signed long long` to `double`
+// does not raise the error.
+// So we use it for a workaround.
+#if defined (__i386__) && defined (__APPLE__) && \
+  defined (__SSE2_MATH__) && __GNUC__ < 5
+{
+  I64 inum = num_;
+  I64 iden = den_;
+

Re: Issue 5733: Fix various type-conversion warnings (issue 559450053 by nine.fierce.ball...@gmail.com)

[nine . fierce . ballads]

Reviewers: lemzwerg,

Message:
On 2020/02/04 10:45:58, lemzwerg wrote:
> lily/accidental-placement.cc:62: scm_from_long  (stagger ?
context_hash : 1));
> Are you actually trying clang-format?  There's one space too much :-)

I was too lazy to deal with Han-Wen's config file as a patch.  Now that
it's in master, I plan to try it.

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

1: int->size_t in make_script_from_event ()
... and similarly in Fingering_engraver::make_script ().

2: Fix scm_from_... () with wrong types

3: Fix type-conversion warning in Scale
Assert that scale size is less than the max int value.

4: int->vsize in Dot_column

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

Affected files (+23, -26 lines):
  M lily/accidental-placement.cc
  M lily/dot-column.cc
  M lily/fingering-engraver.cc
  M lily/grob-array-scheme.cc
  M lily/include/script-interface.hh
  M lily/scale.cc
  M lily/script-engraver.cc
  M lily/tab-note-heads-engraver.cc


Index: lily/accidental-placement.cc
diff --git a/lily/accidental-placement.cc b/lily/accidental-placement.cc
index 
0ac9fe98bcfcab465d19d1ff80b172827842423f..43b6971b9eb90b41189ee25c77b1ce8b8fbb514f
 100644
--- a/lily/accidental-placement.cc
+++ b/lily/accidental-placement.cc
@@ -56,10 +56,10 @@ Accidental_placement::add_accidental (Grob *me, Grob *a, 
bool stagger, long cont
 return;
 
   a->set_parent (me, X_AXIS);
-  long n = p->get_notename ();
 
   SCM accs = me->get_object ("accidental-grobs");
-  SCM key = scm_cons (scm_from_int (n), scm_from_long  (stagger ? context_hash 
: 1));
+  SCM key = scm_cons (scm_from_int (p->get_notename ()),
+  scm_from_long  (stagger ? context_hash : 1));
   // assoc because we're dealing with pairs
   SCM entry = scm_assoc (key, accs);
   if (scm_is_false (entry))
Index: lily/dot-column.cc
diff --git a/lily/dot-column.cc b/lily/dot-column.cc
index 
744062c0edac9abc0a40d76ccae34b4d5628a5bb..66c9c91c2830ded090224fe1a22402be6b541d36
 100644
--- a/lily/dot-column.cc
+++ b/lily/dot-column.cc
@@ -170,11 +170,11 @@ Dot_column::calc_positioning_done (SCM smob)
   for (vsize j = 0; j < parent_stems.size (); j++)
 {
   Interval chord = Stem::head_positions (parent_stems[j]);
-  int total_room = ((int) chord.length () + 2
-+ scm_to_int (chord_dots_limit)) / 2;
-  int total_dots = dots_each_stem[j].size ();
+  vsize total_room = (static_cast (chord.length ()) + 2
++ scm_to_size_t (chord_dots_limit)) / 2;
+  vsize total_dots = dots_each_stem[j].size ();
   // remove excessive dots from the ends of the stem
-  for (int first_dot = 0; total_dots > total_room; total_dots--)
+  for (vsize first_dot = 0; total_dots > total_room; total_dots--)
 if (0 == (total_dots - total_room) % 2)
   dots_each_stem[j][first_dot++]->suicide ();
 else
Index: lily/fingering-engraver.cc
diff --git a/lily/fingering-engraver.cc b/lily/fingering-engraver.cc
index 
d6a49f2dff55f261d028e849a9ef686f59b0516b..f7b9e68d29e7bf03af74fefb34b1ea08081d2fc4
 100644
--- a/lily/fingering-engraver.cc
+++ b/lily/fingering-engraver.cc
@@ -46,7 +46,7 @@ protected:
   void acknowledge_note_column (Grob_info);
 
 private:
-  void make_script (Direction, Stream_event *, int);
+  void make_script (Direction, Stream_event *, size_t);
 };
 
 void
@@ -100,7 +100,7 @@ Fingering_engraver::process_music ()
 }
 
 void
-Fingering_engraver::make_script (Direction d, Stream_event *r, int i)
+Fingering_engraver::make_script (Direction d, Stream_event *r, size_t i)
 {
   Item *fingering = make_item ("Fingering", r->self_scm ());
 
@@ -111,16 +111,12 @@ Fingering_engraver::make_script (Direction d, 
Stream_event *r, int i)
   Side_position_interface::set_axis (fingering, Y_AXIS);
   Self_alignment_interface::set_aligned_on_parent (fingering, X_AXIS);
 
-  // Hmm
-  int priority = 200;
-  SCM s = fingering->get_property ("script-priority");
-  if (scm_is_number (s))
-priority = scm_to_int (s);
-
   /* See script-engraver.cc */
-  priority += i;
-
-  fingering->set_property ("script-priority", scm_from_int (priority));
+  SCM priority = fingering->get_property ("script-priority");
+  if (!scm_is_number (priority))
+priority = scm_from_int (200); // TODO: Explain magic.
+  priority = scm_sum (priority, scm_from_size_t (i));
+  fingering->set_property ("script-priority", priority);
 
   if (d)
 fingering->set_property ("direction", scm_from_int (d));
Index: lily/grob-array-scheme.cc
diff --git a/lily/grob-array-scheme.cc b/lily/grob-array-scheme.cc
index 
23b6a14174af1f61af143a8528f11b999655aecd..3d9148d97c19e18cfb8b91d714a9a21a33815cc6
 100644
--- a/lily/grob-array-scheme.cc
+++ b/lily/grob-array-scheme.cc
@@ -29,7 +29,7 @@ LY_DEFINE (ly_grob_array_length, "ly:grob-array-length",
   LY_ASSERT_SMOB (Grob_array, grob_arr, 1);
 
   Grob_array *me = unsmob 

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[David Kastrup]

Masamichi Hosoda  writes:

>> We currently have the problem that the compiler used in GUB for
>> compiling 32bit binaries gets an internal compiler fault for those
>> options.
>> 
>> We'll need to figure out whether a newer compiler does the trick, and if
>> it does, update GUB.  Or find a different way of proceeding.
>> 
>> I'll see whether I can convince my current compilers to generate 32bit
>> code and see whether those are currently up to using those options.  If
>> current compilers don't want them, we'll need to revert to a different
>> plan.
>
> It seems that static cast from `unsigned long long` to `double`
> by x86 SSE2 raises the internal compile error.
> However, static cast from `signed long long` to `double`
> does not raise the errir.
> I think it can be a workaround for rational.cc.
>
> i.e.
> First, static cast from `unsigned long long` to `signed long long`
> Then, static cast from `singed long long` to `double`

Oh wow.  I would never have thought one could identify something as
specific.  I think I have convinced my system to build a 32bit Guile,
but have problems convincing LilyPond to do the same.  So I cannot help
with debugging this situation yet or even finding out whether it
persists into newer compiler versions.

-- 
David Kastrup

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[Masamichi Hosoda]

> We currently have the problem that the compiler used in GUB for
> compiling 32bit binaries gets an internal compiler fault for those
> options.
> 
> We'll need to figure out whether a newer compiler does the trick, and if
> it does, update GUB.  Or find a different way of proceeding.
> 
> I'll see whether I can convince my current compilers to generate 32bit
> code and see whether those are currently up to using those options.  If
> current compilers don't want them, we'll need to revert to a different
> plan.

It seems that static cast from `unsigned long long` to `double`
by x86 SSE2 raises the internal compile error.
However, static cast from `signed long long` to `double`
does not raise the errir.
I think it can be a workaround for rational.cc.

i.e.
First, static cast from `unsigned long long` to `signed long long`
Then, static cast from `singed long long` to `double`

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[David Kastrup]

ArnoldTheresius  writes:

> Thomas Morley-2 wrote
>> Am Sa., 1. Feb. 2020 um 16:49 Uhr schrieb David Kastrup 
>
>> dak@
>
>> :
>> ...
>> 
>> Ok, thanks for the explanation.
>> 
>> Iiuc, this would make this patch superfluous and it should be abandoned.
>> As I'm only shepherding it, I'll wait for Arnold.
>> For now I'll set the issue to 'waiting' to get it out of Jame's hairs for
>> now.
>> 
>> Tbh, I have no clue how to implement a -mfpmath=sse -msse2 option myself.
>> 
>> Thanks,
>>   Harm
>> 
>> P.S.
>> Just seen the patch at
>> https://codereview.appspot.com/565600045/
>
> Hello,
>
> check (examination routine) with the other command line options all work
> fine:
> a) -mfpmath=sse -msse2

We currently have the problem that the compiler used in GUB for
compiling 32bit binaries gets an internal compiler fault for those
options.

We'll need to figure out whether a newer compiler does the trick, and if
it does, update GUB.  Or find a different way of proceeding.

I'll see whether I can convince my current compilers to generate 32bit
code and see whether those are currently up to using those options.  If
current compilers don't want them, we'll need to revert to a different
plan.

> b) -march=pentium4 -mfpmath=sse -msse2
> c) -march=pentium4 -mfpmath=sse -msse2 -Wl,--large-address-aware
>
> But, if no one can tell, how to set this command line option for compilation
> in GUB,
> I vote to continue this patch, and add a TODO comment next to the inline
> assembler call.
>
> ArnoldTheresius

To quote from Phil's report (from which one can also glean the current
version in use, at least in the file paths)

I would expect flower/rational.cc to be one of the earliest files GUB
tries to compile, so this might not be a rare occurence but a principal
problem with this compiler version and those options.

/home/gub/NewGub/gub/target/darwin-x86/src/lilypond-git.sv.gnu.org--lilypond.git-stable-2.20/flower/rational.cc:43:1:
internal compiler error: in gen_reg_rtx, at emit-rtl.c:838
}
^
librestrict:error:/home/gub/NewGub/gub/target/darwin-x86/root/usr/cross/libexec/gcc/i686-apple-darwin8/4.9.4/cc1plus:
tried to open () file 
/home/gub/NewGub/gub/target/tools/root/usr/lib/librestrict.so
librestrict:allowed:
/home/gub/NewGub/gub/target/darwin-x86
/tmp
/dev/null
/dev/urandom
/proc/self

/home/gub/NewGub/gub/target/darwin-x86/src/lilypond-git.sv.gnu.org--lilypond.git-stable-2.20/flower/rational.cc:43:1:
internal compiler error: Aborted
librestrict:error:/home/gub/NewGub/gub/target/darwin-x86/root/usr/cross/libexec/gcc/i686-apple-darwin8/4.9.4/cc1plus:
tried to open () file 
/home/gub/NewGub/gub/target/tools/root/usr/lib/librestrict.so
librestrict:allowed:
/home/gub/NewGub/gub/target/darwin-x86
/tmp
/dev/null
/dev/urandom
/proc/self
{standard input}:unknown:Undefined local symbol
L__ZNSs4_Rep10_M_destroyERKSaIcE$stub
i686-apple-darwin8-g++: internal compiler error: Aborted (program cc1plus)
librestrict:error:/home/gub/NewGub/gub/target/darwin-x86/root/usr/cross/bin/i686-apple-darwin8-g++:
tried to open () file 
/home/gub/NewGub/gub/target/tools/root/usr/lib/librestrict.so
librestrict:allowed:
/home/gub/NewGub/gub/target/darwin-x86
/tmp
/dev/null
/dev/urandom
/proc/self
Aborted (core dumped)
make[1]: *** [out/rational.o] Error 134
make[1]: Leaving directory
`/home/gub/NewGub/gub/target/darwin-x86/build/lilypond-git.sv.gnu.org--lilypond.git-stable-2.20/flower'
make: *** [all] Error 2
Command barfed: cd
/home/gub/NewGub/gub/target/darwin-x86/build/lilypond-git.sv.gnu.org--lilypond.git-stable-2.20
&& make -j16 TARGET_PYTHON="/usr/bin/env python"


-- 
David Kastrup

GUB fail

[Phil Holmes]

David K warned me this morning that my attempt to build 2.19.84 might fail 
with a compiler error.  It did.  The error output is:


/home/gub/NewGub/gub/target/darwin-x86/src/lilypond-git.sv.gnu.org--lilypond.git-stable-2.20/flower/rational.cc:43:1: 
internal compiler error: in gen_reg_rtx, at emit-rtl.c:838

}
^
librestrict:error:/home/gub/NewGub/gub/target/darwin-x86/root/usr/cross/libexec/gcc/i686-apple-darwin8/4.9.4/cc1plus: 
tried to open () file 
/home/gub/NewGub/gub/target/tools/root/usr/lib/librestrict.so

librestrict:allowed:
/home/gub/NewGub/gub/target/darwin-x86
/tmp
/dev/null
/dev/urandom
/proc/self

/home/gub/NewGub/gub/target/darwin-x86/src/lilypond-git.sv.gnu.org--lilypond.git-stable-2.20/flower/rational.cc:43:1: 
internal compiler error: Aborted
librestrict:error:/home/gub/NewGub/gub/target/darwin-x86/root/usr/cross/libexec/gcc/i686-apple-darwin8/4.9.4/cc1plus: 
tried to open () file 
/home/gub/NewGub/gub/target/tools/root/usr/lib/librestrict.so

librestrict:allowed:
/home/gub/NewGub/gub/target/darwin-x86
/tmp
/dev/null
/dev/urandom
/proc/self
{standard input}:unknown:Undefined local symbol 
L__ZNSs4_Rep10_M_destroyERKSaIcE$stub

i686-apple-darwin8-g++: internal compiler error: Aborted (program cc1plus)
librestrict:error:/home/gub/NewGub/gub/target/darwin-x86/root/usr/cross/bin/i686-apple-darwin8-g++: 
tried to open () file 
/home/gub/NewGub/gub/target/tools/root/usr/lib/librestrict.so

librestrict:allowed:
/home/gub/NewGub/gub/target/darwin-x86
/tmp
/dev/null
/dev/urandom
/proc/self
Aborted (core dumped)
make[1]: *** [out/rational.o] Error 134
make[1]: Leaving directory 
`/home/gub/NewGub/gub/target/darwin-x86/build/lilypond-git.sv.gnu.org--lilypond.git-stable-2.20/flower'

make: *** [all] Error 2
Command barfed: cd 
/home/gub/NewGub/gub/target/darwin-x86/build/lilypond-git.sv.gnu.org--lilypond.git-stable-2.20 
&& make -j16 TARGET_PYTHON="/usr/bin/env python"




--
Phil Holmes

Issue 5733: Fix various type-conversion warnings (issue 559450053 by nine.fierce.ball...@gmail.com)

[lemzwerg--- via Discussions on LilyPond development]

LGTM


https://codereview.appspot.com/559450053/diff/547570044/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

https://codereview.appspot.com/559450053/diff/547570044/lily/accidental-placement.cc#newcode62
lily/accidental-placement.cc:62: scm_from_long  (stagger ? context_hash
: 1));
Are you actually trying clang-format?  There's one space too much :-)

https://codereview.appspot.com/559450053/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[dak]

On 2020/02/04 08:56:49, hanwenn wrote:
> On 2020/02/03 12:11:08, dak wrote:
> > > What is wrong with the code I propose in this change?
> > 
> > You mean define-syntax?  I'd like to avoid it for now since people
have no way
> > of getting acquainted with it, and most new developers have never
worked with
> > Scheme before.
> 
> I don't understand this argument. If we have no new developers that
know Scheme,
> they wouldn't know what to do with defmacro either? Am I missing
something?

defmacro works in user documents.  define-syntax doesn't.  That gives
people a way to get acquainted with defmacro that doesn't exist yet for
define-syntax .

> I didn't know about define-syntax before writing this patch, and I
learned by 
> googling around for it.



https://codereview.appspot.com/553480044/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[hanwenn]

On 2020/02/03 12:11:08, dak wrote:
> > What is wrong with the code I propose in this change?
> 
> You mean define-syntax?  I'd like to avoid it for now since people
have no way
> of getting acquainted with it, and most new developers have never
worked with
> Scheme before.

I don't understand this argument. If we have no new developers that know
Scheme,
they wouldn't know what to do with defmacro either? Am I missing
something? 

I didn't know about define-syntax before writing this patch, and I
learned by 
googling around for it.


https://codereview.appspot.com/553480044/

Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)

[ArnoldTheresius]

Thomas Morley-2 wrote
> Am Sa., 1. Feb. 2020 um 16:49 Uhr schrieb David Kastrup 

> dak@

> :
> ...
> 
> Ok, thanks for the explanation.
> 
> Iiuc, this would make this patch superfluous and it should be abandoned.
> As I'm only shepherding it, I'll wait for Arnold.
> For now I'll set the issue to 'waiting' to get it out of Jame's hairs for
> now.
> 
> Tbh, I have no clue how to implement a -mfpmath=sse -msse2 option myself.
> 
> Thanks,
>   Harm
> 
> P.S.
> Just seen the patch at
> https://codereview.appspot.com/565600045/

Hello,

check (examination routine) with the other command line options all work
fine:
a) -mfpmath=sse -msse2
b) -march=pentium4 -mfpmath=sse -msse2
c) -march=pentium4 -mfpmath=sse -msse2 -Wl,--large-address-aware

But, if no one can tell, how to set this command line option for compilation
in GUB,
I vote to continue this patch, and add a TODO comment next to the inline
assembler call.

ArnoldTheresius



--
Sent from: http://lilypond.1069038.n5.nabble.com/Dev-f88644.html