On 2020/05/09 12:47:29, Dan Eble wrote: > https://codereview.appspot.com/576090043/diff/569740045/lily/staff-symbol.cc > File lily/staff-symbol.cc (right): > > https://codereview.appspot.com/576090043/diff/569740045/lily/staff-symbol.cc#newcode116 > lily/staff-symbol.cc:116: return ly_scm2floatvector (line_positions); > The suggested patch to expose internal_line_count() as line_count() bypasses > this branch.
Well, only in multi-measure-rest.cc (that’s the only place where only the line count is used, not the full vector). I do, however, worry about the comment before (what used to be) the internal_line_count() definition: // Get the line-count property directly. This is for internal use when it is // known that the line-positions property is not relevant. Could there be any situations where the line-count SCM property and the line_positions vector length would differ? > line_count() should have the same branches as line_positions() but > call scm_ilength() (I think) in this branch, and internal_line_count() in the > other branch. I don’t understand what you’re referring to. The point of my latest patch is that there’s no longer an internal_line_count(). The only (publicly exposed) line_count() function that remains is inferred only from the line-count property, as explained in the comment above. The other point is that it now bypasses the vector’s creation altogether when avoidable, whereas what you’re suggesting (IIUC, which I probably don’t) would not. > It isn't a lot of code, but it's the kind of thing that could easily get out of > sync with line_positions() when later contributors come by. If I were to proceed like you’re suggesting, it may. But my proposal was much simpler, as it just uses the existing internal_line_count() function. (Again, I may not understand exactly what you’re suggesting, so please feel free to help me out.) > It's not something > I'd want to commit myself, but if you are interested in doing it, I'm willing to > say the L-word once it's ready. Heh. Didn’t they used to make a TV show about that? :-) Cheers, -- V. https://codereview.appspot.com/576090043/
