Issue 7300082: Eliminates pure-print-callbacks list (Closed)
|
Created:
11 years, 2 months ago by MikeSol Modified:
11 years, 1 month ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionEliminates pure-print-callbacks list
Patch Set 1 #
Total comments: 2
Patch Set 2 : Changes name of stencil height function #Patch Set 3 : Renames yet again #
Total comments: 2
Patch Set 4 : Adds comment #MessagesTotal messages: 14
https://codereview.appspot.com/7300082/diff/1/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/7300082/diff/1/scm/define-grobs.scm#newcode2570 scm/define-grobs.scm:2570: (Y-extent . ,pure-safe-stencil-height) Are there grobs that don't have this definition of Y-extent? If it is the same everywhere, do we even need an entry for it? https://codereview.appspot.com/7300082/diff/1/scm/output-lib.scm File scm/output-lib.scm (right): https://codereview.appspot.com/7300082/diff/1/scm/output-lib.scm#newcode61 scm/output-lib.scm:61: (define-public pure-safe-stencil-height Perhaps add any information about the name? Its name claims to produce a pure value, but it actually outputs callbacks both for pure and unpure. What makes it "safe"?
Sign in to reply to this message.
On 11 févr. 2013, at 16:29, dak@gnu.org wrote: > > https://codereview.appspot.com/7300082/diff/1/scm/define-grobs.scm > File scm/define-grobs.scm (right): > > https://codereview.appspot.com/7300082/diff/1/scm/define-grobs.scm#newcode2570 > scm/define-grobs.scm:2570: (Y-extent . ,pure-safe-stencil-height) > Are there grobs that don't have this definition of Y-extent? Yes. > If it is > the same everywhere, do we even need an entry for it? > > https://codereview.appspot.com/7300082/diff/1/scm/output-lib.scm > File scm/output-lib.scm (right): > > https://codereview.appspot.com/7300082/diff/1/scm/output-lib.scm#newcode61 > scm/output-lib.scm:61: (define-public pure-safe-stencil-height > Perhaps add any information about the name? Its name claims to produce > a pure value, but it actually outputs callbacks both for pure and > unpure. What makes it "safe"? maybe unpure-pure-stencil-height is better? > > https://codereview.appspot.com/7300082/
Sign in to reply to this message.
On 2013/02/11 17:01:17, mike7 wrote: > On 11 févr. 2013, at 16:29, mailto:dak@gnu.org wrote: > > scm/output-lib.scm:61: (define-public pure-safe-stencil-height > > Perhaps add any information about the name? Its name claims to produce > > a pure value, but it actually outputs callbacks both for pure and > > unpure. What makes it "safe"? > > maybe unpure-pure-stencil-height is better? Its effect would presumably be a callback that is not replaced by its value when called. Correct? All that pure/unpure whatever is just waving internals around for a concept that has little to do with it. I'd use something like (ly:retriggerable-callback ly:grob::stencil-height) for that, and since you use it a whole lot of time and presumably don't want to have one closure per use, you can use something like (define ly:grob::retriggerable-stencil-height (ly:retriggerable-callback ly:grob::stencil-height)) once in a useful location and then use that. Your coding style only ever shows mechanisms, not concepts. That makes the code about as pleasant and easy to read as disassembled machine code. I am not sure I got the concept right here: that's your job. Not that of the reader or reviewer.
Sign in to reply to this message.
Changes name of stencil height function
Sign in to reply to this message.
Renames yet again
Sign in to reply to this message.
It does looks reasonable to put the promise that a stencil does not change with line-spacing, near the definiton fo the stencil, rather than in a separate list. Just one question. If we override the stencil \override NoteHead #'stencil = #stencil-notehead then formerly LilyPond would avoid drawing it until after note-spacing, giving an empty extent to note-spacing, so we would provide an estimated extent like \override NoteHead #'minimum-Y-extent = #'(-0.5 . 0.5) for use in spacing of notes and accidentals. Now it seems the default will be that a user's stencil *is* evaluated at the note-spacing stage, if the stencil it replaces was safe to evaluate at that time. Is that what you wanted? https://codereview.appspot.com/7300082/diff/4002/scm/define-grobs.scm File scm/define-grobs.scm (left): https://codereview.appspot.com/7300082/diff/4002/scm/define-grobs.scm#oldcode... scm/define-grobs.scm:2728: (define pure-print-callbacks This could have had a comment: "These print routines do not depend on the spacing of a line, and we require that they do not depend on the spacing of a line, so they can be evaluated to determine the space needed at the note-spacing stage." https://codereview.appspot.com/7300082/diff/4002/scm/output-lib.scm File scm/output-lib.scm (right): https://codereview.appspot.com/7300082/diff/4002/scm/output-lib.scm#newcode60 scm/output-lib.scm:60: ;; Using this as a callback for a grob's Y-extent promises ;; that the grob's stencil does not depend on line-spacing. ;; We use this promise to figure the space required by Clefs ;; and such at the note-spacing stage.
Sign in to reply to this message.
Adds comment
Sign in to reply to this message.
On 17 févr. 2013, at 00:16, k-ohara5a5a@oco.net wrote: > It does looks reasonable to put the promise that a stencil does not > change with line-spacing, near the definiton fo the stencil, rather than > in a separate list. > > Just one question. If we override the stencil > \override NoteHead #'stencil = #stencil-notehead > then formerly LilyPond would avoid drawing it until after note-spacing, > giving an empty extent to note-spacing, so we would provide an estimated > extent like > \override NoteHead #'minimum-Y-extent = #'(-0.5 . 0.5) > for use in spacing of notes and accidentals. > > Now it seems the default will be that a user's stencil *is* evaluated at > the note-spacing stage, if the stencil it replaces was safe to evaluate > at that time. Is that what you wanted? Yes. > > > https://codereview.appspot.com/7300082/diff/4002/scm/define-grobs.scm > File scm/define-grobs.scm (left): > > https://codereview.appspot.com/7300082/diff/4002/scm/define-grobs.scm#oldcode... > scm/define-grobs.scm:2728: (define pure-print-callbacks > This could have had a comment: "These print routines do not depend on > the spacing of a line, and we require that they do not depend on the > spacing of a line, so they can be evaluated to determine the space > needed at the note-spacing stage." > > https://codereview.appspot.com/7300082/diff/4002/scm/output-lib.scm > File scm/output-lib.scm (right): > > https://codereview.appspot.com/7300082/diff/4002/scm/output-lib.scm#newcode60 > scm/output-lib.scm:60: > ;; Using this as a callback for a grob's Y-extent promises > ;; that the grob's stencil does not depend on line-spacing. > ;; We use this promise to figure the space required by Clefs > ;; and such at the note-spacing stage. > > https://codereview.appspot.com/7300082/ Comment added. Cheers, MS
Sign in to reply to this message.
On 2013/02/11 17:15:47, dak wrote: > On 2013/02/11 17:01:17, mike7 wrote: > > On 11 févr. 2013, at 16:29, mailto:dak@gnu.org wrote: > > > > scm/output-lib.scm:61: (define-public pure-safe-stencil-height > > > Perhaps add any information about the name? Its name claims to produce > > > a pure value, but it actually outputs callbacks both for pure and > > > unpure. What makes it "safe"? > > > > maybe unpure-pure-stencil-height is better? > > Its effect would presumably be a callback that is not replaced by its value when > called. Correct? All that pure/unpure whatever is just waving internals around > for a concept that has little to do with it. > > I'd use something like > (ly:retriggerable-callback ly:grob::stencil-height) > for that, and since you use it a whole lot of time and presumably don't want to > have one closure per use, you can use something like > > (define ly:grob::retriggerable-stencil-height > (ly:retriggerable-callback ly:grob::stencil-height)) > > once in a useful location and then use that. Your coding style only ever shows > mechanisms, not concepts. That makes the code about as pleasant and easy to > read as disassembled machine code. I am not sure I got the concept right here: > that's your job. Not that of the reader or reviewer. Is there a particular reason you ignored this comment, judging from the commit you pushed to staging on your own initiative?
Sign in to reply to this message.
On 20 févr. 2013, at 11:35, dak@gnu.org wrote: > On 2013/02/11 17:15:47, dak wrote: >> On 2013/02/11 17:01:17, mike7 wrote: >> > On 11 févr. 2013, at 16:29, mailto:dak@gnu.org wrote: > >> > > scm/output-lib.scm:61: (define-public pure-safe-stencil-height >> > > Perhaps add any information about the name? Its name claims to > produce >> > > a pure value, but it actually outputs callbacks both for pure and >> > > unpure. What makes it "safe"? >> > >> > maybe unpure-pure-stencil-height is better? > >> Its effect would presumably be a callback that is not replaced by its > value when >> called. Correct? All that pure/unpure whatever is just waving > internals around >> for a concept that has little to do with it. > >> I'd use something like >> (ly:retriggerable-callback ly:grob::stencil-height) >> for that, and since you use it a whole lot of time and presumably > don't want to >> have one closure per use, you can use something like > >> (define ly:grob::retriggerable-stencil-height >> (ly:retriggerable-callback ly:grob::stencil-height)) > >> once in a useful location and then use that. Your coding style only > ever shows >> mechanisms, not concepts. That makes the code about as pleasant and > easy to >> read as disassembled machine code. I am not sure I got the concept > right here: >> that's your job. Not that of the reader or reviewer. > > Is there a particular reason you ignored this comment, judging from the > commit you pushed to staging on your own initiative? I changed the name of the function twice in two successive patch sets to better reflect what is going on. Cheers, MS
Sign in to reply to this message.
"mike@mikesolomon.org" <mike@mikesolomon.org> writes: > On 20 févr. 2013, at 11:35, dak@gnu.org wrote: > >> Is there a particular reason you ignored this comment, judging from the >> commit you pushed to staging on your own initiative? > > I changed the name of the function twice in two successive patch sets > to better reflect what is going on. It's currently grob::all-heights-from-stencil. That suggests different types of height. You could call this a review error of mine: I've simply gotten too exhausted to look again closely. The saving grace is that there is actually a comment for the definition now. ;; Using this as a callback for a grob's Y-extent promises ;; that the grob's stencil does not depend on line-spacing. ;; We use this promise to figure the space required by Clefs ;; and such at the note-spacing stage. (define-public grob::all-heights-from-stencil (ly:make-unpure-pure-container ly:grob::stencil-height (lambda (grob start end) (ly:grob::stencil-height grob)))) Now we all know that comments are a scarce resource and should see as much use of possible. So how about following through with my suggestion and creating a creating function and documenting that? (define-public (constant-grob-callback callback) "Wraps a grob @var{callback} into an unpure-pure-container delivering the same results for pure and unpure calculations. This means that the result of the callback must not depend on the line spacing." (ly:make-unpure-pure-container callback (lambda (grob start end) (callback grob)))) Then look-and-behold, we can define (define-public grob::constant-height-from-stencil (constant-grob-callback grob::stencil-height)) Now look-and-behold, we can get along without a separate comment for each such definition since constant-grob-callback already tells us all that we need to know (by virtue of its comment). Is that where we can stop? Actually, no. If we take a look at ly:make-unpure-pure-container, we see LY_DEFINE (ly_make_unpure_pure_container, "ly:make-unpure-pure-container", 1, 1, 0, (SCM unpure, SCM pure), "Make an unpure-pure container. @var{unpure} should be an unpure" " expression, and @var{pure} should be a pure expression. If @var{pure}" " is ommitted, the value of @var{unpure} will be used twice.") Of course, "an unpure expression" and "a pure expression" is pure gobbledygook not even having any tangible meaning, since an expression is not pure or unpure but rather gets used in a particular context. Anyway, we already _have_ an API for using the same value in an unpure-pure-container twice. Why don't we _use_ it? The superficial answer is that pure and unpure callbacks are called in a different manner: the unpure callback gets more arguments (or was that the other way round? Since the unpure/pure naming is not really related to what this does, I can't remember). So in the case of a callback, we can't just reuse the callback like we do with fixed values. Right? Wrong. All we need to do is changing the line if (pure == SCM_UNDEFINED) pure = unpure; in the definition of ly_make_unpure_pure_container into if (pure == SCM_UNDEFINED && !scm_is_procedure (unpure)) pure = unpure; and then let ly_unpure_pure_container_unpure_part and/or ly_unpure_pure_container_pure_part deal with the situation of having only one procedure with a single argument stored by concocting a three-argument procedure on-the-fly (or change all callers of the callback procedure to deal with the one-argument situation for half-full containers. That makes the change less encapsulated by making external stuff responsible for dealing with half-full containers, but may be conceptually simpler). And lo-and-behold, all we need to write as a callback _directly_ is (ly:make-unpure-pure-container ly:grob::stencil-height) and that's it. Now apart from the drawback that either your ly:grob::all-heights-from-stencil or the purported constant-grob-callback functions are much better documented than ly:make-unpure-pure-container, this is quite more readable and obvious because it works the same way with callbacks as with fixed values: specify only the pure and the unpure will follow suit. We have the advantage that we don't have to look for the best name for an additional function because the existing functions can perfectly well be made to do the job. Yes, I should have thought of that previously, but it is really exhausting to do this kind of review. And frankly, I still don't understand why this is not equivalent to ly:grob::stencil-height anyway. But at least this is not opening yet another can of worms but just making do with the already open ones. -- David Kastrup
Sign in to reply to this message.
On 20 févr. 2013, at 14:06, David Kastrup <dak@gnu.org> wrote: > > (define-public (constant-grob-callback callback) > "Wraps a grob @var{callback} into an unpure-pure-container > delivering the same results for pure and unpure calculations. This > means that the result of the callback must not depend on the line > spacing." > (ly:make-unpure-pure-container > callback > (lambda (grob start end) (callback grob)))) > > Then look-and-behold, we can define > > (define-public grob::constant-height-from-stencil > (constant-grob-callback grob::stencil-height)) > > Now look-and-behold, we can get along without a separate comment for > each such definition since constant-grob-callback already tells us all > that we need to know (by virtue of its comment). > This is an excellent idea - hadn't crossed my mind. Please propose a patch if you have time. > Is that where we can stop? Actually, no. If we take a look at > ly:make-unpure-pure-container, we see > > LY_DEFINE (ly_make_unpure_pure_container, "ly:make-unpure-pure-container", > 1, 1, 0, (SCM unpure, SCM pure), > "Make an unpure-pure container. @var{unpure} should be an unpure" > " expression, and @var{pure} should be a pure expression. If @var{pure}" > " is ommitted, the value of @var{unpure} will be used twice.") > > Of course, "an unpure expression" and "a pure expression" is pure > gobbledygook not even having any tangible meaning, since an expression > is not pure or unpure but rather gets used in a particular context. > > Anyway, we already _have_ an API for using the same value in an > unpure-pure-container twice. Why don't we _use_ it? The superficial > answer is that pure and unpure callbacks are called in a different > manner: the unpure callback gets more arguments (or was that the other > way round? Since the unpure/pure naming is not really related to what > this does, I can't remember). So in the case of a callback, we can't > just reuse the callback like we do with fixed values. > > Right? Wrong. All we need to do is changing the line > > if (pure == SCM_UNDEFINED) > pure = unpure; > > in the definition of ly_make_unpure_pure_container into > > if (pure == SCM_UNDEFINED && !scm_is_procedure (unpure)) > pure = unpure; > > and then let ly_unpure_pure_container_unpure_part and/or > ly_unpure_pure_container_pure_part deal with the situation of having > only one procedure with a single argument stored by concocting a > three-argument procedure on-the-fly (or change all callers of the > callback procedure to deal with the one-argument situation for half-full > containers. That makes the change less encapsulated by making external > stuff responsible for dealing with half-full containers, but may be > conceptually simpler). > > And lo-and-behold, all we need to write as a callback _directly_ is > > (ly:make-unpure-pure-container ly:grob::stencil-height) > > and that's it. Now apart from the drawback that either your > ly:grob::all-heights-from-stencil or the purported > constant-grob-callback functions are much better documented than > ly:make-unpure-pure-container, this is quite more readable and obvious > because it works the same way with callbacks as with fixed values: > specify only the pure and the unpure will follow suit. > Also a great idea. > We have the advantage that we don't have to look for the best name for > an additional function because the existing functions can perfectly well > be made to do the job. > > Yes, I should have thought of that previously, but it is really > exhausting to do this kind of review. > > And frankly, I still don't understand why this is not equivalent to > ly:grob::stencil-height anyway. But at least this is not opening yet > another can of worms but just making do with the already open ones. I'm proposing another patch set in a day or so that eliminates the rest of the lists in define-grobs.scm and does everything via unpure-pure-containers. After that I'll have a better idea if this is feasible. Cheers, MS
Sign in to reply to this message.
"mike@mikesolomon.org" <mike@mikesolomon.org> writes: > On 20 févr. 2013, at 14:06, David Kastrup <dak@gnu.org> wrote: > >> >> (define-public (constant-grob-callback callback) >> "Wraps a grob @var{callback} into an unpure-pure-container >> delivering the same results for pure and unpure calculations. This >> means that the result of the callback must not depend on the line >> spacing." >> (ly:make-unpure-pure-container >> callback >> (lambda (grob start end) (callback grob)))) >> >> Then look-and-behold, we can define >> >> (define-public grob::constant-height-from-stencil >> (constant-grob-callback grob::stencil-height)) >> >> Now look-and-behold, we can get along without a separate comment for >> each such definition since constant-grob-callback already tells us all >> that we need to know (by virtue of its comment). >> > > This is an excellent idea - hadn't crossed my mind. It is _exactly_ what I proposed in <URL:https://codereview.appspot.com/7300082#msg3> in review. > Please propose a patch if you have time. There is no point in doing so. As mentioned further down, the whole thing can be obliterated by just extending unpure-pure-containers in a natural way. And anyway, you state > I'm proposing another patch set in a day or so that eliminates the > rest of the lists in define-grobs.scm and does everything via > unpure-pure-containers. After that I'll have a better idea if this is > feasible. So that would just be conflicting commits. And I really fail to see the point in me proposing patches to clean up after you when I already invested the work of proposing fixes in review. Where is the point in reviewing the stuff, proposing fixes, and then having to clean up anything by oneself anyway? If you are not interested in maintaining your own code, how can you expect anybody else to be? -- David Kastrup
Sign in to reply to this message.
On 2013/02/16 22:16:28, Keith wrote: > It does looks reasonable to put the promise that a stencil does not change with > line-spacing, near the definiton fo the stencil, rather than in a separate list. See <URL:http://code.google.com/p/lilypond/issues/detail?id=3200> for a more transparent way to put this promise for callbacks than defining yet another inventively named container.
Sign in to reply to this message.
|
