Code review - Issue 197690044: Function to display the rhythmic location of a grobhttps://codereview.appspot.com/2015-02-22T20:26:01+00:00rietveld
Message from unknown
2015-02-18T17:51:41+00:00david.nalesnikurn:md5:a9910e2d585b0e2d523e52ac1b51711f
Message from david.nalesnik@gmail.com
2015-02-18T17:53:44+00:00david.nalesnikurn:md5:95c03d7a95d60bcbb9ca9175c326afce
Please review. Thanks!
Message from dak@gnu.org
2015-02-18T18:02:38+00:00dakurn:md5:6562ca966b2bd1f644be1f3ecf0f4e97
On 2015/02/18 17:53:44, david.nalesnik wrote:
> Please review. Thanks!
Can't this be done just as easily in Scheme?
Message from david.nalesnik@gmail.com
2015-02-18T18:05:17+00:00david.nalesnikurn:md5:b417feb0684e31b1d39032a4c157d55b
On 2015/02/18 18:02:38, dak wrote:
> On 2015/02/18 17:53:44, david.nalesnik wrote:
> > Please review. Thanks!
>
> Can't this be done just as easily in Scheme?
Sure--that crossed my mind. The reason I went with C++ is so that it would be documented prominently, but I could change it if that's not a sufficient reason.
Message from dak@gnu.org
2015-02-18T18:07:19+00:00dakurn:md5:380c2b3059ff013c4f8ae1a5388e4758
On 2015/02/18 18:05:17, david.nalesnik wrote:
> On 2015/02/18 18:02:38, dak wrote:
> > On 2015/02/18 17:53:44, david.nalesnik wrote:
> > > Please review. Thanks!
> >
> > Can't this be done just as easily in Scheme?
>
> Sure--that crossed my mind. The reason I went with C++ is so that it would be
> documented prominently, but I could change it if that's not a sufficient reason.
It should not be a sufficient reason. Document-string it just as you would the C++ version, and we'll need to come up with a good way to pull those into the internals manual anyway.
Message from david.nalesnik@gmail.com
2015-02-18T18:08:32+00:00david.nalesnikurn:md5:58c795e056d924ac233dd28b865cc8b5
On 2015/02/18 18:07:19, dak wrote:
> On 2015/02/18 18:05:17, david.nalesnik wrote:
> > On 2015/02/18 18:02:38, dak wrote:
> > > On 2015/02/18 17:53:44, david.nalesnik wrote:
> > > > Please review. Thanks!
> > >
> > > Can't this be done just as easily in Scheme?
> >
> > Sure--that crossed my mind. The reason I went with C++ is so that it would be
> > documented prominently, but I could change it if that's not a sufficient
> reason.
>
> It should not be a sufficient reason. Document-string it just as you would the
> C++ version, and we'll need to come up with a good way to pull those into the
> internals manual anyway.
OK, will do.
Message from david.nalesnik@gmail.com
2015-02-18T18:12:58+00:00david.nalesnikurn:md5:962fdfced3b62b938790dffd6164b7ff
On 2015/02/18 18:08:32, david.nalesnik wrote:
Putting it into Scheme will need to wait for Issue 4289 to make it to master, since I'll be able to use ly:item-get-column here.
Message from david.nalesnik@gmail.com
2015-02-18T20:39:19+00:00david.nalesnikurn:md5:ae1bb29bbf3caf03ecac1c499c5799b7
On 2015/02/18 18:12:58, david.nalesnik wrote:
> On 2015/02/18 18:08:32, david.nalesnik wrote:
>
> Putting it into Scheme will need to wait for Issue 4289 to make it to master,
> since I'll be able to use ly:item-get-column here.
However, we lose the nice error reporting which happens when you do (ly:grob-name 2) for example.
Message from david.nalesnik@gmail.com
2015-02-18T20:43:51+00:00david.nalesnikurn:md5:91187c80b07e6e65c77ca327a12b3bbe
On 2015/02/18 20:39:19, david.nalesnik wrote:
> On 2015/02/18 18:12:58, david.nalesnik wrote:
> > On 2015/02/18 18:08:32, david.nalesnik wrote:
> >
> > Putting it into Scheme will need to wait for Issue 4289 to make it to master,
> > since I'll be able to use ly:item-get-column here.
>
> However, we lose the nice error reporting which happens when you do
> (ly:grob-name 2) for example.
Arghh, Comment attached to wrong issue. But the same applies: it seems that a drawback of using Scheme is losing the nice error messages that result from doing LY_ASSERT_SMOB.
Message from unknown
2015-02-19T20:22:33+00:00david.nalesnikurn:md5:f825b9b00be45bf041cc20187bb91da0
Message from david.nalesnik@gmail.com
2015-02-19T20:37:35+00:00david.nalesnikurn:md5:a1ff6583d913205f881688b941aaacb2
On 2015/02/18 20:43:51, david.nalesnik wrote:
>
> it seems that a
> drawback of using Scheme is losing the nice error messages that result from
> doing LY_ASSERT_SMOB.
False alarm.
Message from paulwmorris@gmail.com
2015-02-22T03:01:04+00:00pwmurn:md5:485023d339e72ff2e473a1e3d2075582
LGTM, except for a couple minor things. Thanks for working on these scheme functions.
https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm
File scm/output-lib.scm (right):
https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode30
scm/output-lib.scm:30: "Return the rhythmic position of grob @var{g} as a list. The
Do you mean @var{grob} ? That would be more in line with other functions in this file.
https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode42
scm/output-lib.scm:42: (list when rl))
Instead of this let block I'd probably just do:
(list (ly:grob-property col 'when)
(ly:grob-property col 'rhythmic-location))
But either way is fine with me.
Message from dak@gnu.org
2015-02-22T07:59:40+00:00dakurn:md5:9eabe72f52ff4e24f6630d63869c731e
https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm
File scm/output-lib.scm (right):
https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode29
scm/output-lib.scm:29: (define-public (grob::rhythmic-location grob)
The function is misnamed since rhythmic-location is an established term in the code base with a number of operators that will not work on the result from this function. A "rhythmic location" is just the cadr of what you return here.
So you should split this function into two, one for returning the global timestep (there probably is an established term for that, but if not, grob::when would be an obvious candidate), and one for returning the rhythmic location.
There is likely some duplication of code then, but since it will be the exception rather than rule that one needs both values, the resulting code will likely be more rather than less efficient for the typical use case.
https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode30
scm/output-lib.scm:30: "Return the rhythmic position of grob @var{g} as a list. The
On 2015/02/22 03:01:04, pwm wrote:
> Do you mean @var{grob} ? That would be more in line with other functions in
> this file.
It doesn't matter as much whether it would be more in line with other functions (though it is of course a consideration), but as written currently it is not even in line with the actual function argument name.
Message from unknown
2015-02-22T19:30:46+00:00david.nalesnikurn:md5:47d527d9f5d7dcffbbc662bc6e8e8c30
Message from david.nalesnik@gmail.com
2015-02-22T20:21:25+00:00david.nalesnikurn:md5:5cd09a7dbec6309e7b615e5209f59895
https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm
File scm/output-lib.scm (right):
https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode29
scm/output-lib.scm:29: (define-public (grob::rhythmic-location grob)
On 2015/02/22 07:59:40, dak wrote:
> The function is misnamed since rhythmic-location is an established term in the
> code base with a number of operators that will not work on the result from this
> function. A "rhythmic location" is just the cadr of what you return here.
>
> So you should split this function into two, one for returning the global
> timestep (there probably is an established term for that, but if not, grob::when
> would be an obvious candidate), and one for returning the rhythmic location.
>
> There is likely some duplication of code then, but since it will be the
> exception rather than rule that one needs both values, the resulting code will
> likely be more rather than less efficient for the typical use case.
Done.
https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode30
scm/output-lib.scm:30: "Return the rhythmic position of grob @var{g} as a list. The
On 2015/02/22 03:01:04, pwm wrote:
> Do you mean @var{grob} ? That would be more in line with other functions in
> this file.
Done.
https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode42
scm/output-lib.scm:42: (list when rl))
On 2015/02/22 03:01:04, pwm wrote:
> Instead of this let block I'd probably just do:
>
> (list (ly:grob-property col 'when)
> (ly:grob-property col 'rhythmic-location))
>
> But either way is fine with me.
Done--in a way!
Message from unknown
2015-02-22T20:26:01+00:00david.nalesnikurn:md5:2e967c7b8361782afee3ffea4b3b4927