Re: Figured Bass extenders syntax
2011/7/26 Reinhold Kainhofer : > PS: I like the idea of using ties for the extenders, because that's exactly > what an extender is meant to express! +1! +1! ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: GOP-PROP 6: private mailing lists (probable decision)
2011/7/27 Graham Percival : > Most people seem to like the status quo. > > http://lilypond.org/~graham/gop/gop_6.html > > ** Proposal summary > > Potentially sensitive or private matters will be referred to > Graham. He will then decide who should discuss the matter on an > ad-hoc basis, and forward or CC them on future emails. > > For emphasis, the project administrators are Han-Wen, Jan, and > Graham; those three will always be CC’d on any important > discussions. > > The lilypond-hackers mailing list will be removed. I'm fine with this. cheers, Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: music function semantics
Neil Puttock writes: > On 26 July 2011 22:41, David Kastrup wrote: > >> So the question basically is: which of those mechanisms is actually >> being in use? Are there examples for existing music functions >> interpreting a postevent or a chord constituent? > > \tweak would be the most common usage for both of these cases: > > c1-\tweak #'color #red -\fermata > > and > > < \tweak #'color #red c>1 So much for my "nobody needs that" theory. The problem I have is that accepting \transpose in all the same places as \tweak does not seem like a good idea. On the other hand, whether an error gets thrown by the parser or by the expression builder might not make that much of a difference to the end user than it feels like making to me. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: font: change breve vertical lines (issue4748044)
http://codereview.appspot.com/4748044/diff/1/mf/feta-noteheads.mf File mf/feta-noteheads.mf (right): http://codereview.appspot.com/4748044/diff/1/mf/feta-noteheads.mf#newcode181 mf/feta-noteheads.mf:181: % when there is an interval of fourth. Wouldn't it be simpler to explicitly specify the line length instead of using such a complicated algorithm? I mean a global variable to be added to the various feta-noteheadsXX.mf files. I know that none of the existing size driver files contain such a variable except the standard assignment of `design_size', but this shouldn't stop you to add something since it would be the logical place to add. Cf. similar control files in other MF fonts like TeX's Computer Modern, which are full of such meta-variables. Han-Wen? http://codereview.appspot.com/4748044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068)
http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode131 lily/multi-measure-rest.cc:131: int measure_duration_log = int (ceil (duration_log)); To me, having done numerical work, the chain of functions you use, int( ceil( -log2(x))), is easily recognizable as "find the smallest n so that 1 / 2^n <= x". While log2 is inexact, the results when x is a power of 2 are exact (IEEE-compliant floating-point represents integers and integer powers of 2 exactly) so reliably 3/4 and 2/4 both come out 1 (half/minim rest). http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode132 lily/multi-measure-rest.cc:132: if (round && duration_log - measure_duration_log < 0) This, however, makes me pause, then wonder why you didn't use the same idiom if (to_boolean(me->get_property ("round-to-longer-rest")) measure_duration_log = int (floor (duration_log)); else measure_duration_log = int (ceil (duration_log)); http://codereview.appspot.com/4536068/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068)
On 2011/07/26 22:47:53, Bertrand Bordage wrote: > > For example, what to do about 3/2? We either get 2/1 rests, or 1/1 rests, and > it is not clear to me that this rounding choice is necessarily the same as that > for 3/4. > > I think it might be saner to [...] This sounds complex... How it works now is a little complex, but reasonable. Both 3/4 and 2/4 have measure_duration_log=1 ask for half/minim rests, rounding to shorter, but these are not on the default usable-duration-logs, so we use the closest usable which are whole/semibreve. Correct. 3/2 has measure_duration_log=0 ask for whole rests by default. If I want have my 3/2 music use breve multi-measure rests, I can round-to-longer-rest=##t. If I want my 4/2 music to use whole/semibreve rests (more likely) I need to cut some entries from usable-duration-logs -- I hope the Notation Reference teaches me how to do this. P.S. Bummer about the missing log2() in freeBSD; they seem to have a tracked bug about it. http://codereview.appspot.com/4536068/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
GOP-PROP 6: private mailing lists (probable decision)
Most people seem to like the status quo. http://lilypond.org/~graham/gop/gop_6.html ** Proposal summary Potentially sensitive or private matters will be referred to Graham. He will then decide who should discuss the matter on an ad-hoc basis, and forward or CC them on future emails. For emphasis, the project administrators are Han-Wen, Jan, and Graham; those three will always be CC’d on any important discussions. The lilypond-hackers mailing list will be removed. ** Status quo At the moment, this seems to be our custom. Whenever something comes up, somebody sends me a private email, and I pick an ad-hoc collection of people to discuss it with. Always Han-Wen and Jan, but often Carl, Trevor, and others. Other than the obvious “giving git push ability”, recent questions included a university project who wanted to have a focus group to discuss development. I thought we could just discuss it on -devel, but the university group wanted to keep it private. I didn’t see any harm in that, so we arranged something privately with an ad-hoc collection of lilypond developers. ** History There is some unhappy history about this idea in our development community: http://lists.gnu.org/archive/html/lilypond-devel/2010-09/msg00178.html http://news.lilynet.net/spip.php?article121 http://lists.gnu.org/archive/html/lilypond-devel/2010-11/msg00076.html ** Other projects The idea of private mailing lists is hardly uncommon in open-source software. For example, http://lwn.net/Articles/394660/ about debian-private http://subversion.apache.org/mailing-lists.html private@ http://www.freebsd.org/administration.html#t-core http://foundation.gnome.org/legal/ board members pledge to keep certain matters confidential every security team of every linux distribution and OS In fact, Karl Fogel’s “Producing Open Source Software” explicitly suggests a private mailing list for some circumstances: [on granting commit/push access to a contributor] But here is one of the rare instances where secrecy is appropriate. You can't have votes about potential committers posted to a public mailing list, because the candidate's feelings (and reputation) could be hurt. http://producingoss.com/en/consensus-democracy.html#electorate ** Board of governers, voting, etc? Many projects have an official board of directors, or a list of “core developers”, with set term limits and elections and stuff. I don’t think that we’re that big. I think we’re still small enough, and there’s enough trust and consensus decisions, that we can avoid that. I would rather that we kept on going with trust+consensus for at least the next 2-3 years, and spent more time+energy on bug fixes and new features instead of administrative stuff. Project administrators are Han-Wen, Jan, and Graham. ** Implementation notes Graham’s email address will be added to the website “contact” page, at the bottom of the “Developer discussion” box, with the caption: Private matters should be sent to Graham Percival, who will discuss it with those concerned. Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc build error
On Tue, Jul 26, 2011 at 09:58:14PM +0100, Phil Holmes wrote: > I'm _still_ running the older version of lilydev, but with all the > patches. hmm. > It's really quite an odd problem: the recipe should be in > texinfo-rules.make: > > $(outdir)/%.texi: $(src-dir)/%.texi > cp -p $< $@ > > But when I put a warning in that recipe, I don't get it on the > errored language. That sounds like exactly the problem I saw. I never figured out why it happened; I just know that it didn't happen when I used the updated lilydev. My intuition is that it's not worth investigating it, but if you think it might be a good learning experience, go for it. Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068)
http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode130 lily/multi-measure-rest.cc:130: double duration_log = -log2 (ml.Rational::to_double ()); log2 is a floating point operation not guaranteed to be exact. One usually uses a loop for figuring out a proper integer value. It is also not clear to me that this will pick out the right kind of rest always. For example, what to do about 3/2? We either get 2/1 rests, or 1/1 rests, and it is not clear to me that this rounding choice is necessarily the same as that for 3/4. I think it might be saner to just have an overrideable lookup list for exact meters (and 4/4 is not necessarily the same as 2/2 here), and revert to a separately configurable default otherwise, likely a whole rest. This sounds complex... I agree with you, but I don't know if others will since it adds a heavy autobeaming-like system. I need another day (or couple of days) to implement this feature. Anyway, I made all the other changes. Feel free to critisize, I still need to learn much about C/Scheme/LilyPond. Bertrand http://codereview.appspot.com/4536068/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: music function semantics
On 26 July 2011 22:41, David Kastrup wrote: > So the question basically is: which of those mechanisms is actually > being in use? Are there examples for existing music functions > interpreting a postevent or a chord constituent? \tweak would be the most common usage for both of these cases: c1-\tweak #'color #red -\fermata and < \tweak #'color #red c>1 Cheers, Neil ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
music function semantics
Hi, I am currently stuck in rethinking what music functions should be able to do. Actually, the point is more that I am _tracing_ that, and the current state is just scary in its inconsistency, which is only alleviated by being pretty much undocumented. So I am not sure which of the undocumented "features" is actually being used. Currently music functions can occur in three places. Basically standalone as prefix_composite_music, inside of a chord as music_function_chord_body, and as an event modifier as music_function_event. prefix_composite_music may take an arbitrary number of ly:music? arguments, but music_function_chord_body and music_function_event may only have the last argument be a music event, and then music_function_chord_body will get the next chord body event as last argument, and music_function_event will get the following music event attached to the current event. So if we define a music function taking just one music expression as argument, it may end up in any of the three places, getting different music types as argument, and being allowed to return different music types depending on where it is used. Total mess syntactically. If we want to turn something like \transpose into a music function, it is not likely we want to have it accepted in all of those places. And we probably won't care for the error messages we get when it is used on a postevent. So the question basically is: which of those mechanisms is actually being in use? Are there examples for existing music functions interpreting a postevent or a chord constituent? -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Lilypond-book: Implement musicxml support in lilypond-book (issue1659041)
A few minor syntax and doc policy issues http://codereview.appspot.com/1659041/diff/5001/Documentation/usage/lilypond-book.itely File Documentation/usage/lilypond-book.itely (right): http://codereview.appspot.com/1659041/diff/5001/Documentation/usage/lilypond-book.itely#newcode206 Documentation/usage/lilypond-book.itely:206: @itemize @bullet Please see Contributor's guide 5.3.6 Syntax survey Lists We don't use @bullet with @itemize. http://codereview.appspot.com/1659041/diff/5001/Documentation/usage/lilypond-book.itely#newcode207 Documentation/usage/lilypond-book.itely:207: @item the @code{\lilypond@{...@}} command, where you can directly enter short lilypond code On 2011/07/26 15:49:50, lemzwerg wrote: Please stay within a width of 80 characters. 80? 72 (preferably 66 - see 5.3.5 Text formatting in CG - also see @item in the various sections) http://codereview.appspot.com/1659041/diff/5001/Documentation/usage/lilypond-book.itely#newcode226 Documentation/usage/lilypond-book.itely:226: @end example I did this @example ... @end example or @example .. @end example or @example ... @end example Myself a few days ago on another section and Graham, quite rightly, said it was unnecessary. Can we just use one single @example ... @end example for all of these? http://codereview.appspot.com/1659041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
RE: Doc build error
Hello, From: lilypond-devel-bounces+james.lowe=datacore@gnu.org [lilypond-devel-bounces+james.lowe=datacore@gnu.org] on behalf of Phil Holmes [m...@philholmes.net] Sent: 26 July 2011 21:58 To: Graham Percival Cc: lilypond-devel@gnu.org Subject: Re: Doc build error - Original Message - From: "Graham Percival" To: "Phil Holmes" Cc: Sent: Tuesday, July 26, 2011 8:41 PM Subject: Re: Doc build error > On Tue, Jul 26, 2011 at 04:15:45PM +0100, Phil Holmes wrote: >> I currently can't build doc. Fresh pull, nuked and recreated build >> directory. I get: > > Cannot reproduce with git master > f9af9dd76a0481c993caf90ebefb1b8a71c5898d > > You're not on the lilypond/translation branch by mistake, are you? > > * I don't think that's the case, but I can't think of any other > reason why I can build but you can't -- we're both running > lilydev, right? > > ** oh wait, I now recall some weird kernel bug which manifested > when building on lilydev in virtualbox, but never in lilydev > installed natively. It went away when I did a kernel update, but > maybe ubuntu reintroduced that problem? are you running lilydev > with all the latest security patches applied? > > Cheers, > - Graham > I'm _still_ running the older version of lilydev, but with all the patches. It's really quite an odd problem: the recipe should be in texinfo-rules.make: $(outdir)/%.texi: $(src-dir)/%.texi cp -p $< $@ But when I put a warning in that recipe, I don't get it on the errored language. I'm gonna nuke the whole git directory and start from scratch. If that doesn't work, I'll find out what the problem is. -- I've just done a new doc build with no problems (I didn't see this thread until now). @Phil you should use the out of tree 'build' dir method, it's really convenient - see the CG. ;) James ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
On 26 July 2011 11:17, m...@apollinemike.com wrote: > This is my fault. I don't know why I missed it these warnings in the > side-by-side comparison, but I won't let this slip again. It isn't your fault. There were no warnings. It appears David's getting warnings from the more recent change to rest-ledger.ly (which must be architecture-specific; it compiles cleanly here before and after the change). Cheers, Neil ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
On 26 July 2011 18:43, David Kastrup wrote: > Come on. I got pointed to this patch because _warnings_ occured. Don't > tell me "David is the only one who can see warnings". The patch is in > an area I have no clue about. _Anybody_ with reasonable C and Scheme > experience would have seen the same things I noted. I do not have reasonable C and Scheme experience. I started programming by accident (only to fix bugs on LilyPond; previously my programming experience amounted to nothing more than 'Hello World' in 68k assembler on an Amiga 500, and that's twenty years ago), so it's likely my LGTMs often miss things which may be considered basic errors to more experienced coders. Cheers, Neil ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc build error
- Original Message - From: "Graham Percival" To: "Phil Holmes" Cc: Sent: Tuesday, July 26, 2011 8:41 PM Subject: Re: Doc build error On Tue, Jul 26, 2011 at 04:15:45PM +0100, Phil Holmes wrote: I currently can't build doc. Fresh pull, nuked and recreated build directory. I get: Cannot reproduce with git master f9af9dd76a0481c993caf90ebefb1b8a71c5898d You're not on the lilypond/translation branch by mistake, are you? * I don't think that's the case, but I can't think of any other reason why I can build but you can't -- we're both running lilydev, right? ** oh wait, I now recall some weird kernel bug which manifested when building on lilydev in virtualbox, but never in lilydev installed natively. It went away when I did a kernel update, but maybe ubuntu reintroduced that problem? are you running lilydev with all the latest security patches applied? Cheers, - Graham I'm _still_ running the older version of lilydev, but with all the patches. It's really quite an odd problem: the recipe should be in texinfo-rules.make: $(outdir)/%.texi: $(src-dir)/%.texi cp -p $< $@ But when I put a warning in that recipe, I don't get it on the errored language. I'm gonna nuke the whole git directory and start from scratch. If that doesn't work, I'll find out what the problem is. -- Phil Holmes ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc build error
Le 26/07/2011 21:41, Graham Percival disait : On Tue, Jul 26, 2011 at 04:15:45PM +0100, Phil Holmes wrote: I currently can't build doc. Fresh pull, nuked and recreated build directory. I get: Cannot reproduce with git master f9af9dd76a0481c993caf90ebefb1b8a71c5898d You're not on the lilypond/translation branch by mistake, are you? * I don't think that's the case, but I can't think of any other reason why I can build but you can't -- we're both running lilydev, right? ** oh wait, I now recall some weird kernel bug which manifested when building on lilydev in virtualbox, but never in lilydev installed natively. It went away when I did a kernel update, but maybe ubuntu reintroduced that problem? are you running lilydev with all the latest security patches applied? I got it on my Fedora14, but on the zh directory. Re-invoking make doc has been successful. I'm in sync with 922d59a242646c2488738d77263580427bd7764b I will make another try tomorrow night on a fresh local clone of master. HTH, Jean-Charles ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc build error
On Tue, Jul 26, 2011 at 04:15:45PM +0100, Phil Holmes wrote: > I currently can't build doc. Fresh pull, nuked and recreated build > directory. I get: Cannot reproduce with git master f9af9dd76a0481c993caf90ebefb1b8a71c5898d You're not on the lilypond/translation branch by mistake, are you? * I don't think that's the case, but I can't think of any other reason why I can build but you can't -- we're both running lilydev, right? ** oh wait, I now recall some weird kernel bug which manifested when building on lilydev in virtualbox, but never in lilydev installed natively. It went away when I did a kernel update, but maybe ubuntu reintroduced that problem? are you running lilydev with all the latest security patches applied? Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
On Tue, Jul 26, 2011 at 07:36:43PM +0200, David Kastrup wrote: > > So it needs to tell its story in comments. It doesn't. There is a lot > of code in Lilypond that nonchalantly expects people to get along > without commenting what it does. This is often a nuisance, but if the > code is written by a master, the pain of figuring out what it does is > usually tolerable. Very true; I think that almost all developers (if not all!) would like to see better comments in the code! Could you get into the habit of pointing out questionable / unclear parts of code in future patches, so that the author will explain them in comments? I imagine that the first few weeks would require an awful lot of reviews to get a good level of comments, but over time authors would get used to the amount+type of comments that we want to see. > Masters don't grow on trees. Indeed. We need to nurture non-Masters -- speaking of both people like Carl and Neil (who, although knowing an incredible amount about lilypond, are still not perfect), and people like Janek and Bertrand (who are relatively new contributors). How can we nurture those very different types of people? - more reviews - reviewing their reviews For new contributors, a mentorship system would help, but that requires mentors (which are in short supply). [the patch author] > invested more care and effort than he sees us doing. > > Can we do better? Yes. Do more reviews, and be more careful in reviews. If I could wave a magic wand, I think the first thing I'd ask is for all developers (i.e. people with git push ability) to spend 1 hour a day reviewing patches. That's obviously not going to happen, but let's stumble forward as best we can, with the resources at our disposal. Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
Reinhold Kainhofer writes: > So, it seems the only one who is aware of those rounding problems is > David. The question then is, why didn't David do a review of the patch > and now complains that the process is not working? Come on. I got pointed to this patch because _warnings_ occured. Don't tell me "David is the only one who can see warnings". The patch is in an area I have no clue about. _Anybody_ with reasonable C and Scheme experience would have seen the same things I noted. As Graham said: this patch got nod-offs from several bigwigs. How would I have suspected that a review of mine in code that I have no clue about would have changed anything? i stumbled upon it because the regtests suddenly gave warnings. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
Graham Percival writes: > On Tue, Jul 26, 2011 at 12:05:31PM +0200, David Kastrup wrote: >> "m...@apollinemike.com" writes: > ... >> > Did 104f80daf1dab11ef5b598006e3d4be8dfbe1926 go through full review? >> > Was it verified with a full build? >> >> Very likely. This was not "gold star" quality which tends to work on >> first compilation attempt. > > This patch looks absolutely excellent to me. > > 104f80daf1dab11ef5b598006e3d4be8dfbe1926 > fixed: > http://code.google.com/p/lilypond/issues/detail?id=1655 > first patch set May 17 or so: > http://codereview.appspot.com/4543055/ > next patch set May 26 or so: > http://codereview.appspot.com/4536068/ > this second patchset had a total of: > - 6 drafts > - 9 comments not from the author > - a LGTM from Neil > - countdowns on > June 03 > http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00036.html > June 06 > http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00104.html > June 14 > http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00283.html > > Speaking from an administrative perspective, this patch looked > *excellent* to push. The term "gold star" refers to code that runs on first compilation. It is not a measure of the final quality. I was giving a subjective impression of how "natural" the code appeared to me, and the code does not exhibit the light and consistent hand of a master. It looks like the result of work rather than inspiration. And that means that I am fairly confident it _was_ verified with a full build. Likely several full builds before it compiled without errors. It has a regression test fitting it, to boot. I agree with you that from an administrative perspective, this patch could hardly have been better. From my personal perspective as code wrangler, there are a number of deficiencies in code and execution. Probably worst is that the code does not speak for itself. Often you look at code, and see what it does and why. Not here. So it needs to tell its story in comments. It doesn't. There is a lot of code in Lilypond that nonchalantly expects people to get along without commenting what it does. This is often a nuisance, but if the code is written by a master, the pain of figuring out what it does is usually tolerable. I don't feel confident understanding what this patch does and why. Masters don't grow on trees. So we have, in my view, a large discrepancy between administrative quality and code quality for this patch. And I want to _stress_ that this is _not_, I repeat _not_ the fault of the patch author. He invested more care and effort than he sees us doing. Can we do better? -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
Am Dienstag, 26. Juli 2011, 18:54:28 schrieb David Kastrup: > Graham Percival writes: > > On Tue, Jul 26, 2011 at 12:05:31PM +0200, David Kastrup wrote: > >> Perhaps a minimal measure of sanity would be if a patch countdown > >> without code review was only started when the author of the patch > >> says "I feel reasonably confident that this not just works, but is > >> good". > >> > >> In git, there is the "formal" sanctification of "Signed-off-by". > >> Perhaps we should not start a patch countdown on any patch that has > >> not been signed off by anybody? > > > > I speak against this, at least for now. This is a question of > > balance between support for new contributors (i.e. mentors, of > > which we have far fewer than I would like), amount of available > > reviewers (which is smaller than we would like), and the moral of > > contributors. > > A contributor is free to add "Signed-off-by" himself. I was talking > about patches that not even the contributor feels confident enough about > to undersign it. No, please don't add any more bureaucratic / adminstrative duty for infrequent contributors that barely know git and the command line. If a contributor contributes a patch, his submission is already his signing. Remember, we are not the group of hardcore geeks the Kernel developers are! I don't see a problem in the review process itself. Rather it is a problem that the quality of reviews that you request requires perfect knowledge. I, for example, was not aware of those possible problems with pow. Neil did a review and gave his LGTM. Probably he wasn't aware of integer/double problems either (and his reviews and his LilyPond knowledge can only be described as excellent). So, it seems the only one who is aware of those rounding problems is David. The question then is, why didn't David do a review of the patch and now complains that the process is not working? Cheers, Reinhold -- -- Reinhold Kainhofer, reinh...@kainhofer.com, http://reinhold.kainhofer.com/ * Financial & Actuarial Math., Vienna Univ. of Technology, Austria * http://www.fam.tuwien.ac.at/, DVR: 0005886 * LilyPond, Music typesetting, http://www.lilypond.org ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
Graham Percival writes: > On Tue, Jul 26, 2011 at 12:05:31PM +0200, David Kastrup wrote: > >> Perhaps a minimal measure of sanity would be if a patch countdown >> without code review was only started when the author of the patch >> says "I feel reasonably confident that this not just works, but is >> good". >> >> In git, there is the "formal" sanctification of "Signed-off-by". >> Perhaps we should not start a patch countdown on any patch that has >> not been signed off by anybody? > > I speak against this, at least for now. This is a question of > balance between support for new contributors (i.e. mentors, of > which we have far fewer than I would like), amount of available > reviewers (which is smaller than we would like), and the moral of > contributors. A contributor is free to add "Signed-off-by" himself. I was talking about patches that not even the contributor feels confident enough about to undersign it. > This is a different problem. Am I correct in reading that it > "passed" the regtests but added some warnings? Well, I am rather positive I saw warnings about the referenced rest glyphs not being found (that made me look in the code in the first place). My first contention, that writing "-" for "M" was the culprit was wrong since the glyph lookup explicitly replaces "-" with "M". The messages are Processing `./3c/lily-e6c82577.ly' Parsing... Renaming input to: `rest-ledger.ly' Interpreting music... Preprocessing graphical objects... Calculating line breaks... Drawing systems... rest-ledger.ly:22:2: warning: rest `rests.-1o' not found r rest-ledger.ly:19:2: warning: rest `rests.-1o' not found r \rPos #-4 rest-ledger.ly:19:2: warning: rest `rests.-1o' not found r \rPos #-4 rest-ledger.ly:22:2: warning: rest `rests.-1o' not found r Writing header field `texidoc' to `./3c/lily-e6c82577.texidoc'... Writing ./3c/lily-e6c82577-1.signature Layout output to `./3c/lily-e6c82577.eps'... Layout output to `./3c/lily-e6c82577-1.eps'... Writing ./3c/lily-e6c82577-systems.texi... Writing ./3c/lily-e6c82577-systems.tex... Writing ./3c/lily-e6c82577-systems.count... Writing timing to 3c/lily-e6c82577.profile... So more likely, the appended "o" is at fault. Using Rest::whateveritwasIsuggested would have been smart enough to avoid appending letters not applicable to the really long rests. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
On Tue, Jul 26, 2011 at 12:05:31PM +0200, David Kastrup wrote: > "m...@apollinemike.com" writes: > > > Perhaps this would be a good case study. > > The more interesting question is just how common the circumstances are > under which this happens. ... > > Did 104f80daf1dab11ef5b598006e3d4be8dfbe1926 go through full review? > > Was it verified with a full build? > > Very likely. This was not "gold star" quality which tends to work on > first compilation attempt. This patch looks absolutely excellent to me. 104f80daf1dab11ef5b598006e3d4be8dfbe1926 fixed: http://code.google.com/p/lilypond/issues/detail?id=1655 first patch set May 17 or so: http://codereview.appspot.com/4543055/ next patch set May 26 or so: http://codereview.appspot.com/4536068/ this second patchset had a total of: - 6 drafts - 9 comments not from the author - a LGTM from Neil - countdowns on June 03 http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00036.html June 06 http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00104.html June 14 http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00283.html Speaking from an administrative perspective, this patch looked *excellent* to push. I'd say that only about 5% of pushed patches have received as much scrutiny as this one; if the bar is even higher, then we'd be looking at pushing 4-5 patches per month. Make no mistake -- I think it would be great if highly skilled people spent more time reviewing patches. Some problems still slip through the cracks, and if somebody is willing to spend more time working to avoid such problems, then great! But we will always have *some* problems. The goal should be to minimize or mitigate those problems, not to require so much scrutiny that problems are impossible. > Perhaps a minimal measure of sanity would be if a patch countdown > without code review was only started when the author of the patch says > "I feel reasonably confident that this not just works, but is good". > > In git, there is the "formal" sanctification of "Signed-off-by". > Perhaps we should not start a patch countdown on any patch that has not > been signed off by anybody? I speak against this, at least for now. This is a question of balance between support for new contributors (i.e. mentors, of which we have far fewer than I would like), amount of available reviewers (which is smaller than we would like), and the moral of contributors. > > Did it pass regtests? > > Sure, but with quite suspicious warnings relevant to the test case. This is a different problem. Am I correct in reading that it "passed" the regtests but added some warnings? IMO, that should not be considered to be "passing" the regtests. Of course, at the moment we have some "harmless" warnings in the regtests; it would be great if we could remove all those "harmless" warnings and be stricter about warnings. I would certainly like to enable warning-as-error for the regtests: http://code.google.com/p/lilypond/issues/detail?id=814 Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068)
http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode254 lily/multi-measure-rest.cc:254: Stencil r (musfont->find_by_name ("rests." + to_string (k))); On 2011/07/26 13:08:11, dak wrote: This may calculate a name "rests.-1" instead of the valid "rests.M1". Use Rest::glyph_name instead, though it may also need fixing in that regard. This comment was erroneous: find_by_name actually converts - into M by itself. However, it might still be a good idea to use Rest::glyph_name in order to get a glyph with the right style for the document. http://codereview.appspot.com/4536068/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Sketch for fix of issue 307 (issue4813048)
On 2011/07/26 06:14:47, mike_apollinemike.com wrote: On Jul 26, 2011, at 5:01 AM, mailto:hanw...@gmail.com wrote: > regtest missing. > > > http://codereview.appspot.com/4813048/diff/3001/lily/slur-scoring.cc > File lily/slur-scoring.cc (right): > > http://codereview.appspot.com/4813048/diff/3001/lily/slur-scoring.cc#newcode286 > lily/slur-scoring.cc:286: end_ys[d] += additional_ys + 0.5; // 0.5 = > m4g1c padding over extra encompass. make tweakable? > "magic" > > Do you need the 0.5 at all? The quanting and the shape of the slur > already offer quite some flexibility. > The test case from the tracker doesn't work without the 0.5, which is truly a magic number that represents "that which was necessary to make the test case work and not look ugly." I'll add a regtest later today. Cheers, MS I did some math to be able to scrub the padding & added a regtest. Cheers, MS http://codereview.appspot.com/4813048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc build error
"Phil Holmes" wrote in message news:j0mln8$tvu$1...@dough.gmane.org... I currently can't build doc. Fresh pull, nuked and recreated build directory. I get: extract_texi_filenames.py: Processing out-www/usage.texi writing: /home/phil/lilypond-git/build/./out-www/xref-maps/usage.de.xref-map cp -p web.texi out-www/web.texi cp: cannot stat `web.texi': No such file or directory make[3]: *** [out-www/web.texi] Error 1 make[3]: Leaving directory `/home/phil/lilypond-git/build/Documentation/de' make[2]: *** [WWW-1] Error 2 rm out-www/weblinks.itexi make[2]: Leaving directory `/home/phil/lilypond-git/build/Documentation' make[1]: *** [WWW-1] Error 2 make[1]: Leaving directory `/home/phil/lilypond-git/build' make: *** [doc-stage-1] Error 2 I've not been able to get a clean build for 1-2 days, but wanted to be sure everything was clean from scratch. Anyone else getting problems? I'll investigate the source, but wanted to get a heads up. Mmm. Interesting. Just re-running make doc gives: writing: /home/phil/lilypond-git/build/./out-www/xref-maps/usage.es.xref-map cp -p web.texi out-www/web.texi cp: cannot stat `web.texi': No such file or directory make[3]: *** [out-www/web.texi] Error 1 make[3]: Leaving directory `/home/phil/lilypond-git/build/Documentation/es' make[2]: *** [WWW-1] Error 2 make[2]: Leaving directory `/home/phil/lilypond-git/build/Documentation' make[1]: *** [WWW-1] Error 2 make[1]: Leaving directory `/home/phil/lilypond-git/build' make: *** [doc-stage-1] Error 2 Note that it's the Spanish files causing the problem this time, not the German. The other "interesting" aspect is that we have "cp -p web.texi out-www/web.texi" as the source of the error. If I look for the similar command for non-translated docs, I find: "cp -p /home/phil/lilypond-git/Documentation/web.texi out-www/web.texi". Spot the directory name. Continuing to investigate... -- Phil Holmes Bug Squad ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Lilypond-book: Implement musicxml support in lilypond-book (issue1659041)
LGTM (without testing). http://codereview.appspot.com/1659041/diff/5001/Documentation/usage/lilypond-book.itely File Documentation/usage/lilypond-book.itely (right): http://codereview.appspot.com/1659041/diff/5001/Documentation/usage/lilypond-book.itely#newcode207 Documentation/usage/lilypond-book.itely:207: @item the @code{\lilypond@{...@}} command, where you can directly enter short lilypond code Please stay within a width of 80 characters. http://codereview.appspot.com/1659041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Doc build error
I currently can't build doc. Fresh pull, nuked and recreated build directory. I get: extract_texi_filenames.py: Processing out-www/usage.texi writing: /home/phil/lilypond-git/build/./out-www/xref-maps/usage.de.xref-map cp -p web.texi out-www/web.texi cp: cannot stat `web.texi': No such file or directory make[3]: *** [out-www/web.texi] Error 1 make[3]: Leaving directory `/home/phil/lilypond-git/build/Documentation/de' make[2]: *** [WWW-1] Error 2 rm out-www/weblinks.itexi make[2]: Leaving directory `/home/phil/lilypond-git/build/Documentation' make[1]: *** [WWW-1] Error 2 make[1]: Leaving directory `/home/phil/lilypond-git/build' make: *** [doc-stage-1] Error 2 I've not been able to get a clean build for 1-2 days, but wanted to be sure everything was clean from scratch. Anyone else getting problems? I'll investigate the source, but wanted to get a heads up. -- Phil Holmes Bug Squad ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068)
Okay, I will fix these before the end of the day. Thanks for taking time to do the review ! Bertrand ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068)
I would suggest reverting this patch as "needs work" for now. http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode125 lily/multi-measure-rest.cc:125: SCM sml = dynamic_cast (me)->get_bound (LEFT) There is no good way to get around the cast? http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode130 lily/multi-measure-rest.cc:130: double duration_log = -log2 (ml.Rational::to_double ()); log2 is a floating point operation not guaranteed to be exact. One usually uses a loop for figuring out a proper integer value. It is also not clear to me that this will pick out the right kind of rest always. For example, what to do about 3/2? We either get 2/1 rests, or 1/1 rests, and it is not clear to me that this rounding choice is necessarily the same as that for 3/4. I think it might be saner to just have an overrideable lookup list for exact meters (and 4/4 is not necessarily the same as 2/2 here), and revert to a separately configurable default otherwise, likely a whole rest. http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode140 lily/multi-measure-rest.cc:140: int list_elt = scm_to_int (scm_list_ref (duration_logs_list, scm_from_int (i))); Iterating through a list as if it were an array is a no-no. This makes the operation O(n^2) instead of O(n) and obfuscates what happens. Check other code examples for how to iterate through a list. http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode229 lily/multi-measure-rest.cc:229: for (int i = 0; i < scm_to_int (scm_length (duration_logs_list)); i++) Again: don't loop through a list by indexing it like an array. http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode246 lily/multi-measure-rest.cc:246: length = int (pow (2, -i)); Don't use floating point arithmetic. This is just 2<<-i unless i is positive. One should exit the loop _before_ that happens, otherwise length will be undefined with the integer variant. With the floating point variant, length becomes 0 before exiting the loop with i==1. I doubt that is intentional. http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#newcode254 lily/multi-measure-rest.cc:254: Stencil r (musfont->find_by_name ("rests." + to_string (k))); This may calculate a name "rests.-1" instead of the valid "rests.M1". Use Rest::glyph_name instead, though it may also need fixing in that regard. http://codereview.appspot.com/4536068/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
[PATCH] Implement MusicXML files in lilypond-book
Please review: http://codereview.appspot.com/1659041/ This patch adds support for including MusicXML files into documents processed by lilypond-book. In particular: -) HTML: filename.xml -) TeX: \musicxmlfile[options]{filename.xml} -) Texinfo: @musicxmlfile[options]{filename.xml} Since MusicXML is so verbose, it doesn't make much sense to support inline MusicXML. The snippets are first processed by musicxml2ly and then treated like a normal lilypond snippet in lilypond-book. The only difference is that the original .xml file is linked in the output rather than the intermediary .ly file. I have mainly implemented this so that the MusicXML test suite can directly link to the .xml file files rather than the lilypond files, which are not relevant for xml anyway. I have also extended lilypond-book to allow additional formatters, so the MusicXML test suite can override the formatting of the MusicXML snippets in the output (e.g. because I want to add some to other renderings in the future, too). Cheers, Reinhold -- -- Reinhold Kainhofer, reinh...@kainhofer.com, http://reinhold.kainhofer.com/ * Financial & Actuarial Math., Vienna Univ. of Technology, Austria * http://www.fam.tuwien.ac.at/, DVR: 0005886 * LilyPond, Music typesetting, http://www.lilypond.org ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Figured Bass extenders syntax
Am Dienstag, 26. Juli 2011, 12:33:03 schrieb Bertrand Bordage: > Yes, this probably needs to be discussed during the GLISS. > But maybe we can do something before, because adding the "tilde notation" > doesn't mean that the current syntax must be removed. But implementing both systems is a lot more work in the code than supporting just one syntax. Also, we should really discuss the details before jumping into changing the syntax. Cheers, Reinhold PS: I like the idea of using ties for the extenders, because that's exactly what an extender is meant to express! -- -- Reinhold Kainhofer, reinh...@kainhofer.com, http://reinhold.kainhofer.com/ * Financial & Actuarial Math., Vienna Univ. of Technology, Austria * http://www.fam.tuwien.ac.at/, DVR: 0005886 * LilyPond, Music typesetting, http://www.lilypond.org ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Figured Bass extenders syntax
Yes, this probably needs to be discussed during the GLISS. But maybe we can do something before, because adding the "tilde notation" doesn't mean that the current syntax must be removed. Bertrand ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
On Jul 26, 2011, at 12:05 PM, David Kastrup wrote: > "m...@apollinemike.com" writes: > >> On Jul 26, 2011, at 11:22 AM, Jan Nieuwenhuizen wrote: >> >>> David Kastrup writes: >>> The overall code makes obvious that this has been created by a comparative novice to the programming languages and data structures of Lilypond. He has been doing his best. >>> >>> I think this patch should be reverted, moved to Rietveld, and worked >>> on. > > Possibly. > >> The reason for my pow (2.0,...) is because the push broke make and my >> fix passed regtests. > > My point was that a more experienced person patched something up > afterwards, and the patch was exclusively for passing a regtest, _kept_ > the problematic real arithmetic and other stuff in the surroundings. > > And no: the regtest _still_ gives out warnings for unknown glyphs. If > people ignore warnings in regtests, we will be forced to turn warnings > into errors. > This is my fault. I don't know why I missed it these warnings in the side-by-side comparison, but I won't let this slip again. >> Perhaps this would be a good case study. > > The more interesting question is just how common the circumstances are > under which this happens. > > Perhaps a minimal measure of sanity would be if a patch countdown > without code review was only started when the author of the patch says > "I feel reasonably confident that this not just works, but is good". > > In git, there is the "formal" sanctification of "Signed-off-by". > Perhaps we should not start a patch countdown on any patch that has not > been signed off by anybody? > I completely agree. This is a condition I impose on myself, and I would not at all mind it to be policy (at least for newer developers). >> Did 104f80daf1dab11ef5b598006e3d4be8dfbe1926 go through full review? >> Was it verified with a full build? > > Very likely. This was not "gold star" quality which tends to work on > first compilation attempt. > >> Did it pass regtests? > > Sure, but with quite suspicious warnings relevant to the test case. > >> Is there any chance that a patch could make it through full review and >> still not build on all Unix-based platforms? > > Of course. > >> Normally, before making any change I post a patch for review, but as >> this seemed fairly urgent, I sent an e-mail to devel and "fixed" the >> issue so that the current master would build. > > Your patch was an improvement in some sense, but it also was a missed > opportunity for improvement, masking a larger problem. > I see what you mean - I had not been operating under the assumption that the original was problematic, but was rather in "oh-crap" mode with regards to current master. Cheers, MS ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
"m...@apollinemike.com" writes: > On Jul 26, 2011, at 11:22 AM, Jan Nieuwenhuizen wrote: > >> David Kastrup writes: >> >>> The overall code makes obvious that this has been created by a >>> comparative novice to the programming languages and data structures of >>> Lilypond. He has been doing his best. >> >> I think this patch should be reverted, moved to Rietveld, and worked >> on. Possibly. > The reason for my pow (2.0,...) is because the push broke make and my > fix passed regtests. My point was that a more experienced person patched something up afterwards, and the patch was exclusively for passing a regtest, _kept_ the problematic real arithmetic and other stuff in the surroundings. And no: the regtest _still_ gives out warnings for unknown glyphs. If people ignore warnings in regtests, we will be forced to turn warnings into errors. > Perhaps this would be a good case study. The more interesting question is just how common the circumstances are under which this happens. Perhaps a minimal measure of sanity would be if a patch countdown without code review was only started when the author of the patch says "I feel reasonably confident that this not just works, but is good". In git, there is the "formal" sanctification of "Signed-off-by". Perhaps we should not start a patch countdown on any patch that has not been signed off by anybody? > Did 104f80daf1dab11ef5b598006e3d4be8dfbe1926 go through full review? > Was it verified with a full build? Very likely. This was not "gold star" quality which tends to work on first compilation attempt. > Did it pass regtests? Sure, but with quite suspicious warnings relevant to the test case. > Is there any chance that a patch could make it through full review and > still not build on all Unix-based platforms? Of course. > Normally, before making any change I post a patch for review, but as > this seemed fairly urgent, I sent an e-mail to devel and "fixed" the > issue so that the current master would build. Your patch was an improvement in some sense, but it also was a missed opportunity for improvement, masking a larger problem. I just recently converted "(format string ..." calls into "(format port string ..." calls, and did not just brainlessly use #f for a port, but partly rearranged things in a manner that obliterated unnecessary display calls, replaced map calls with discarded results with for-each, fixed obviously broken code and so on. Basically, while being there anyway, I cleaned up a bit as well. It's obviously far from being as effective as a full review, but every little bit helps. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
On Jul 26, 2011, at 11:22 AM, Jan Nieuwenhuizen wrote: > David Kastrup writes: > >> The overall code makes obvious that this has been created by a >> comparative novice to the programming languages and data structures of >> Lilypond. He has been doing his best. > > Good catch. The patch also > > @@ -208,63 +221,46 @@ Multi_measure_rest::church_rest (Grob *me, Font_metric > *musfont, int measures, > { > SCM mols = SCM_EOL; > > - /* See Wanske pp. 125 */ > int l = measures; > int count = 0; > > removes a reference to literature, which is very bad. Although I agree > that the comment could elaborate a bit on what Wanske says on page 125, > I don't think we want this kind of references removed. > > I think this patch should be reverted, moved to Rietveld, and worked on. > > Jan > The reason for my pow (2.0,...) is because the push broke make and my fix passed regtests. Perhaps this would be a good case study. Did 104f80daf1dab11ef5b598006e3d4be8dfbe1926 go through full review? Was it verified with a full build? Did it pass regtests? Is there any chance that a patch could make it through full review and still not build on all Unix-based platforms? Normally, before making any change I post a patch for review, but as this seemed fairly urgent, I sent an e-mail to devel and "fixed" the issue so that the current master would build. Cheers, MS ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
Jan Nieuwenhuizen writes: > The patch also > removes a reference Not so, it is moved +++ b/scm/define-grobs.scm @@ -1309,6 +1309,8 @@ (staff-position . 0) (stencil . ,ly:multi-measure-rest::print) (thick-thickness . 6.6) + ;; See Wanske pp. 125 + (usable-duration-logs . (0 -1 -2 -3)) (Y-offset . ,ly:staff-symbol-referencer::callback) sorry, my bad. Jan - Jan Nieuwenhuizen | GNU LilyPond http://lilypond.org Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.nl ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: review process not working
David Kastrup writes: > The overall code makes obvious that this has been created by a > comparative novice to the programming languages and data structures of > Lilypond. He has been doing his best. Good catch. The patch also @@ -208,63 +221,46 @@ Multi_measure_rest::church_rest (Grob *me, Font_metric *musfont, int measures, { SCM mols = SCM_EOL; - /* See Wanske pp. 125 */ int l = measures; int count = 0; removes a reference to literature, which is very bad. Although I agree that the comment could elaborate a bit on what Wanske says on page 125, I don't think we want this kind of references removed. I think this patch should be reverted, moved to Rietveld, and worked on. Jan -- Jan Nieuwenhuizen | GNU LilyPond http://lilypond.org Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.nl ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
review process not working
For whatever reason, the review processes are not working. The code in commit 104f80daf1dab11ef5b598006e3d4be8dfbe1926 repeatedly uses floating point arithmetic like int(pow(2.0,...)) where the outcome is not guaranteed to be correct, it does its calculations based on non-existing glyphs ("rests.-1" instead of "rests.M1"), it loops through lists with an O(n^2) algorithm (first calculating list length, then iterating an integer index for accessing it), it has bad loop conditions. The overall code makes obvious that this has been created by a comparative novice to the programming languages and data structures of Lilypond. He has been doing his best. The patch lists Graham as the committer. Mike later contributed a trivial one-line "fix" (exchanging pow(2, ...) with pow(2.0, ...)). What can we do to avoid things like that happening in future? I stumbled upon this because after an unrelated "make check" where I did not bother to adjust the test-baseline, the warnings for the bad glyph names got into my diff. So I discovered one problem by pure accident and sloppiness, and then looked at the code. And frankly, rests.M1 is an accident waiting to happen. It looks to me as if this particular problem is also in Rest::glyph_name (which frankly is the function that should have been called in the first place). -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel