2009/4/18 Carl D. Sorensen <c_soren...@byu.edu>: > > > > On 4/17/09 1:34 PM, "n.putt...@gmail.com" <n.putt...@gmail.com> wrote:
>> http://codereview.appspot.com/41099/diff/1021/52 >> File Documentation/user/expressive.itely (right): >> >> http://codereview.appspot.com/41099/diff/1021/52#newcode634 >> Line 634: @lilypondfile[verbatim,lilyquote,texidoc,doctitle] >> @ignore this unless you're going to run makelsr.py (or create input/lsr >> file by hand) > > In order to build my docs, I copied the file from input/new to input/lsr. > > I thought that the doc build process would get files from input/new if they > didn't exist in input/lsr. > > Can you summarize the process for me? If I want to add a new snippet to > the docs, how should I do it? I'll reply in the other thread once I've digested it properly. >> >> http://codereview.appspot.com/41099/diff/1021/54#newcode4 >> Line 4: texidoc = " >> needs snippet directory tag otherwise makelsr.py will fail >> >> lsrtags = "rhythms, expressive" > > Fixed, thanks. I wasn't sure what tags to use. I think the only place where they're all listed is in makelsr.py. >> >> http://codereview.appspot.com/41099/diff/1021/59 >> File lily/bezier.cc (right): >> >> http://codereview.appspot.com/41099/diff/1021/59#newcode277 >> Line 277: Offset p[CONTROL_COUNT][CONTROL_COUNT]; >> Offset is described in offset.hh as 2d vector, so should use a vector of >> Offsets > > I read offset.hh as a one-d vector (in computer science terms), with two > elements; element [X_AXIS] and element[Y_AXIS], so it represents a point in > two-dimensional space (hence the 2-d vector). > > I want a matrix of offsets here. The first index is the control point > number. The second index is the equation order (3 for the bezier, 2 for the > first interpolation, 1 for the second interpolation, and 0 for the third > interpolation). This usage is correct. If I'd looked in bezier.hh I might have realized before talking nonsense. :) >> >> http://codereview.appspot.com/41099/diff/1021/59#newcode296 >> Line 296: Bezier::extract (Real t_min, Real t_max) >> ensure t_min and t_max stay within bounds? >> >> negative values and >1 produce outlandish curves >> > > OK, so I think I need some help here. I'm perfectly willing to check the > bounds and give a warning message if the normal bounds aren't met. But I'm > not sure how to do that. Can you give me a pointer to how to do it, a place > in the code to look for an example, and/or an expression to grep? I don't think a warning's necessary, but you can probably get away with using min () and max (). See line-interface.cc for a few examples. >> http://codereview.appspot.com/41099/diff/1021/62#newcode346 >> Line 346: Lookup::slur (Bezier curve, Real curvethick, Real linethick, >> is passing SCM allowed in lookup.cc? > > Why would it not be? lookup.hh has a prototype for slur: > > static Stencil slur (Bezier controls, Real cthick, Real thick, > SCM dash_definition); > > Is there a problem with this as a prototype? > > The code works. Is it a time bomb waiting to explode on me? Of course not. :) It just struck me as out of place, since none of the methods in lookup.cc uses SCMs. >> >> http://codereview.appspot.com/41099/diff/1021/62#newcode351 >> Line 351: /* calculate the offset for the two beziers that make the >> sandwich >> tidy comments > > Is this right? > > /* > calculate the offset for the two beziers that make the sandwich > for the slur > */ lgtm >> >> http://codereview.appspot.com/41099/diff/1021/62#newcode370 >> Line 370: for (int i=0; i<num_segments; i++) >> i < num_segments >> >> should print warning if segments overlap, or just silently ignore? >> > > I thought about it, and decided to allow segments to overlap. I'm not > sure if that was just because I'm lazy, or if I really thought there might > be some use for overlapping segments. Fair enough. It doesn't seem to do any harm anyway. >> >> http://codereview.appspot.com/41099/diff/1021/66 >> File ly/property-init.ly (right): >> >> http://codereview.appspot.com/41099/diff/1021/66#newcode13 >> Line 13: #(define (make-simple-dash-definition dash-fraction >> dash-period) >> this doesn't belong here >> >> http://codereview.appspot.com/41099/diff/1021/66#newcode19 >> Line 19: slurDashPattern = >> these should go in music-functions-init.ly > > Why should these go in music-functions-init.ly instead of property-init.ly? > I'm not disagreeing with you, I'm asking for the criterion for choosing. All the entries in property-init.ly (apart from pointAndClickOn/Off which should also be moved) are simple identifiers which mainly involve constant property settings. >> >> http://codereview.appspot.com/41099/diff/1021/67 >> File python/convertrules.py (right): >> >> http://codereview.appspot.com/41099/diff/1021/67#newcode2903 >> Line 2903: if (re.search(r'\'dash-fraction', str) or >> re.search(r'\'dash-period', str): >> limit search to Slur/Tie/PhrasingSlur otherwise users will get this >> message for other grobs > > does re.search("(Slur|Tie)\s+\'dash-fraction", str) look right? You need a hash before the escaped quote, otherwise it looks OK. One thing I forgot to mention: the NOT_SMART macro expects a sentence continuation rather than a new sentence. >> >> http://codereview.appspot.com/41099/diff/1021/68 >> File scm/define-grob-properties.scm (right): >> >> http://codereview.appspot.com/41099/diff/1021/68#newcode174 >> Line 174: (dash-definition ,pair? "List of @code{dash-elements} defining >> the >> list? > > Han-Wen suggested to me in the past that list? is a very expensive operator > because it must check every element in the list to assure that it's properly > formed. He suggested that unless such a syntax check is needed, it's better > to use pair?, so that's what I've done here. Isn't this a special case though, since it's the type check for a backend property? As far as I can see, all the user properties which take lists use list?, otherwise the IR docs would display the wrong type. > Thanks for the careful review, and for what it's taught me about the > LilyPond standards. You're welcome. Regards, Neil _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
