|
|
Created:
14 years, 5 months ago by dak Modified:
13 years, 1 month ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionMake define-builtin-markup{,-list}-command #:category #:properties keywords
The specification of category and properties makes the *-builtin-*
variants diverge syntactically from the user specified markup. Moving
those specifications into keyword arguments makes the builtin defining
macros upwards compatible with the user specified ones.
All markup commands defined with these macros are adapted to the
new syntax.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address documentation bug and make defining macros work with DOC-string less definitions. #Patch Set 3 : Fix bad treating of DOC string in define-builtin-markup-list-command #Patch Set 4 : Get rid of define-builtin-markup-{,list}-command altogether #Patch Set 5 : This is an attempt to deal with the reported memory leak problem that I can't reproduce myself #Patch Set 6 : Use weak hash tables to avoid memory leakage #
Total comments: 10
Patch Set 7 : Implement -dmake-index option. Minimal time&memory savings: is this a good idea? #Patch Set 8 : Fix module system description in CG to better match current situation. #
Total comments: 10
MessagesTotal messages: 15
David, Thank you for posting this on Rietveld. It was much easier for me to review it. I have one documentation question (see below). I also have one patch philosophy question: should define-markup also have (at least) a :properties keyword added to it? I think that if it did, then we'd have the possibility of using the exact same code and just changing from define-markup to define-builtin-markup by only changing the macro name. Thanks again for posting on Rietveld. It was very helpful for me. Carl http://codereview.appspot.com/160048/diff/1/5 File scm/markup.scm (right): http://codereview.appspot.com/160048/diff/1/5#newcode74 scm/markup.scm:74: [ :category category ] Does this need to be [ #:category category ] ?
Sign in to reply to this message.
Carl.D.Sorensen@gmail.com writes: > David, > > Thank you for posting this on Rietveld. It was much easier for me to > review it. > > I have one documentation question (see below). > > I also have one patch philosophy question: should define-markup also > have (at least) a :properties keyword added to it? Goal #1 was to be able to move user-level markup to system markup unchanged. This goal actually is not achieved with the current state of the patch series since it fails for markup definitions without DOC string. > I think that if it did, then we'd have the possibility of using the > exact same code and just changing from define-markup to > define-builtin-markup by only changing the macro name. After fixing the DOC string issue and exporting define-builtin-markup-command as define-markup-command, the regression test seems to work fine (namely, it runs until it crashes on "Unable to load the map file" which it always does here) and the scoping would appear to be correct as well. Which suggests that the apparent main reason for *-builtin-* commands, the changed toplevel scoping, is not valid, and the scoping works out well enough without extra complications. Renaming define-builtin-markup-command to define-markup-command, making it public and adapting all callers would appear to finish the job, obviously unifying the syntax. I have to check that this is indeed the case. > http://codereview.appspot.com/160048/diff/1/5 > File scm/markup.scm (right): > > http://codereview.appspot.com/160048/diff/1/5#newcode74 > scm/markup.scm:74: [ :category category ] > Does this need to be [ #:category category ] ? Yes. Sorry. -- David Kastrup
Sign in to reply to this message.
http://codereview.appspot.com/160048/diff/1/5 File scm/markup.scm (right): http://codereview.appspot.com/160048/diff/1/5#newcode74 scm/markup.scm:74: [ :category category ] On 2009/11/24 11:01:59, Carl wrote: > Does this need to be [ #:category category ] ? Done.
Sign in to reply to this message.
Hi, Maybe this patch, as a first step, should focus on the unification of the syntax of the two macros? (define-markup-command (command-name layout props . arguments) arguments-signature #:properties property-bindings #:allow-other-keys #:rest body) on the user side and: (define-builtin-markup-command (command-name layout props . arguments) arguments-signature #:category doc-category #:properties property-bindings #:rest body) on LilyPond side. It would be a plus to make the #:category argument mandatory on the builtin side (e.g. by giving it a default value that raises an error). That argument would just be ignored on the user side (by #:allow-other-keys). What do you think? Nicolas
Sign in to reply to this message.
On 2009/11/26 09:13:58, nicolas.sceaux wrote: > Hi, > > Maybe this patch, as a first step, should focus on the unification of the syntax > of the two macros? Hi Nicolas, patch set 3 already makes make-builtin-markup-command upwards-compatible with make-markup-command in its current form. I have not tampered with make-markup-command at all because I don't want to invest work in an approach with code duplication. So I am first trying to get a hang of the problem that the code duplication and the toplevel module definition are trying to solve. That's where I would actually have a chance to learn something. If you feel like taking the work from patch set 3 yourself, go ahead. But it may be less work to wait and see where I get with my attempts. All the best.
Sign in to reply to this message.
Hi, When defining a user markup command, it would be better not to modify at all the variables from the (lily) module, even if you took care of the memory leak. Also, there is one file to be deleted, and the associated \include to be removed from an init .ly file. Nicolas http://codereview.appspot.com/160048/diff/4008/6002 File ly/markup-init.ly (left): http://codereview.appspot.com/160048/diff/4008/6002#oldcode5 ly/markup-init.ly:5: %%;; to be define later, in a closure If this file is empty, then it shall be deleted http://codereview.appspot.com/160048/diff/4008/6007 File scm/markup.scm (right): http://codereview.appspot.com/160048/diff/4008/6007#newcode101 scm/markup.scm:101: (set! body (cddr body))) Why is this needed? Could #:allow-other-keys take care of that? http://codereview.appspot.com/160048/diff/4008/6007#newcode131 scm/markup.scm:131: ;; Register the new function, for markup documentation I still have the feeling that user defined commands should not modify variables from (lily) module. Could there be e.g. a module check, so that the macro expand into the documentation related settings only for builtin commands? http://codereview.appspot.com/160048/diff/4008/6007#newcode168 scm/markup.scm:168: (set! body (cddr body))) Same as above. http://codereview.appspot.com/160048/diff/4008/6007#newcode199 scm/markup.scm:199: (hashq-set! markup-list-functions ,command-name #t) Same as for define-markup-command
Sign in to reply to this message.
Thanks for looking this over. This patch set is sort of a moving target due to the removal of markup-init.ly. For example, recently documentation about the module related definitions in markup-init.ly has been added to the programmer's documentation. If removing markup-init.ly can be done successfully, this documentation could go as well. Unfortunately I still have no feedback about whether the last iteration of dealing with the "memory leak" situation has been successful for those who could see the problems. If that could be corroborrated, investing the time for tracelessly removing markup-init.ly would be a happier task. http://codereview.appspot.com/160048/diff/4008/6002 File ly/markup-init.ly (left): http://codereview.appspot.com/160048/diff/4008/6002#oldcode5 ly/markup-init.ly:5: %%;; to be define later, in a closure On 2009/12/03 14:58:38, nicolas.sceaux wrote: > If this file is empty, then it shall be deleted Agreed. http://codereview.appspot.com/160048/diff/4008/6007 File scm/markup.scm (right): http://codereview.appspot.com/160048/diff/4008/6007#newcode101 scm/markup.scm:101: (set! body (cddr body))) On 2009/12/03 14:58:38, nicolas.sceaux wrote: > Why is this needed? > Could #:allow-other-keys take care of that? defmacro*, in spirit with Common Lisp's keyword arguments, does not remove keywords from the "rest" argument. Which is rather stupid, but not in our hands. This loop just throws any keyword/value pairs from the start of the body away without actually looking at them. This is feasible exactly because #:allow-other-keys has _not_ been set, so we are sure that all keyword/value pairs we throw away have already been legitimately parsed. Do you want a code comment here? Proposal for the wording? http://codereview.appspot.com/160048/diff/4008/6007#newcode131 scm/markup.scm:131: ;; Register the new function, for markup documentation On 2009/12/03 14:58:38, nicolas.sceaux wrote: > I still have the feeling that user defined commands should not modify variables > from (lily) module. > Could there be e.g. a module check, so that the macro expand into the > documentation related settings only for builtin commands? I think it would be more transparent if the documentation selection was turned off completely by default, and a documentation generating run explicitly turned it on. If a user wants to make a small manual for his private markups, being not allowed to do so because of not being in the lily module would be a nuisance. As would be having all lily markups in this manual. A reasonably easy interface would be to use hashq-set! only when the respective entity happens to be a hashtable, or even anything other than #f. I have not yet done anything in that respect since I have no idea yet where to properly place the code turning on/off the documentation collection. http://codereview.appspot.com/160048/diff/4008/6007#newcode168 scm/markup.scm:168: (set! body (cddr body))) On 2009/12/03 14:58:38, nicolas.sceaux wrote: > Same as above. See above. http://codereview.appspot.com/160048/diff/4008/6007#newcode199 scm/markup.scm:199: (hashq-set! markup-list-functions ,command-name #t) On 2009/12/03 14:58:38, nicolas.sceaux wrote: > Same as for define-markup-command See above.
Sign in to reply to this message.
On 12/3/09 9:14 AM, "dak@gnu.org" <dak@gnu.org> wrote: > http://codereview.appspot.com/160048/diff/4008/6007#newcode131 > scm/markup.scm:131: ;; Register the new function, for markup > documentation > On 2009/12/03 14:58:38, nicolas.sceaux wrote: >> I still have the feeling that user defined commands should not modify > variables >> from (lily) module. >> Could there be e.g. a module check, so that the macro expand into the >> documentation related settings only for builtin commands? > > I think it would be more transparent if the documentation selection was > turned off completely by default, and a documentation generating run > explicitly turned it on. > > If a user wants to make a small manual for his private markups, being > not allowed to do so because of not being in the lily module would be a > nuisance. As would be having all lily markups in this manual. > > A reasonably easy interface would be to use hashq-set! only > when the respective entity happens to be a hashtable, or even anything > other than #f. > > I have not yet done anything in that respect since I have no idea yet > where to properly place the code turning on/off the documentation > collection. > How about scm/document-markup.scm? Carl
Sign in to reply to this message.
On 2009/12/03 17:34:24, c_sorensen_byu.edu wrote: > > > > > I have not yet done anything in that respect since I have no idea yet > > where to properly place the code turning on/off the documentation > > collection. > > > > How about scm/document-markup.scm? AFAICS scm/document-markup.scm is only loaded on documentation runs, and then last. I don't see how it could be used for switching documentation collection either on or off. When it is loaded at all, all information must already have been collected.
Sign in to reply to this message.
On 2009/12/03 22:26:04, dak wrote: > On 2009/12/03 17:34:24, c_sorensen_byu.edu wrote: > > > > How about scm/document-markup.scm? > > AFAICS scm/document-markup.scm is only loaded on documentation runs, and then > last. I don't see how it could be used for switching documentation collection > either on or off. When it is loaded at all, all information must already have > been collected. Currently I don't see a better way forward than using a special command line option, something like -dindex-markup on documentation runs. As far as I can see, everything else would come too late in the load order. Nicolas' proposal to only index inside of the lily module would work, but it is rather intransparent to the user and I would not want to seal the possibility to let the user index his own markup, anf just his own. Since there is no point in the collection overhead on non-documentation runs, I think it ok if, unlike now, the default is not to collect any indexing info. If I get no objections, I'll try making a patch for that.
Sign in to reply to this message.
Le 5 déc. 2009 à 09:48, dak@gnu.org a écrit : > Currently I don't see a better way forward than using a special command > line option, something like -dindex-markup on documentation runs. As > far as I can see, everything else would come too late in the load order. > Nicolas' proposal to only index inside of the lily module would work, > but it is rather intransparent to the user and I would not want to seal > the possibility to let the user index his own markup, anf just his own. > Since there is no point in the collection overhead on non-documentation > runs, I think it ok if, unlike now, the default is not to collect any > indexing info. > > If I get no objections, I'll try making a patch for that. Using a LilyPond option was also what I was about to propose, so please go ahead. Nicolas
Sign in to reply to this message.
On 2009/12/05 10:52:52, nicolas.sceaux wrote: > Le 5 déc. 2009 à 09:48, mailto:dak@gnu.org a écrit : > > > Currently I don't see a better way forward than using a special command > > line option, something like -dindex-markup on documentation runs. > > If I get no objections, I'll try making a patch for that. > > Using a LilyPond option was also what I was about to propose, so > please go ahead. Well, patch set 7 does that, but after rethinking this, I suppose I'd call it a mistake. Even if the option gets implemented for other constructs than markup, the time and memory savings are likely negligible. Checking for the option is likely causing more overhead than registering some markup. This would only start making sense if all other indexing operations (not just markup) were suppressed except on documentation runs. I don't think thta significant savings would be the result, and this would completely leave the scope of this particular patchset and its aim. So my take would be to apply patch set 6, and possibly file a TODO item "just index markup, macros, whatever on documentation runs" with low priority that may refer to this discussion and/or patch set 7. A complete job would have to mess with indexing in C code if I am not mistaken. And there is no sense in a half-baked job like that of patch set 7 since markup is just a small part of the job.
Sign in to reply to this message.
On 2009/12/17 11:23:04, dak wrote: > Well, patch set 7 does that, but after rethinking this, I suppose I'd call it a > mistake. Even if the option gets implemented for other constructs than markup, > the time and memory savings are likely negligible. Checking for the option is > likely causing more overhead than registering some markup. Another way to achieve this would have been to do something like (define index-markup-command (if (ly:get-option 'make-index) (lambda ...) ;; return code that indexes the commands (lambda ...))) ;; return empty code and then call the index-markup-command function in the define-markup-command. > So my take would be to apply patch set 6, and possibly file a TODO item "just > index markup, macros, whatever on documentation runs" with low priority that may > refer to this discussion and/or patch set 7. > > A complete job would have to mess with indexing in C code if I am not mistaken. > And there is no sense in a half-baked job like that of patch set 7 since markup > is just a small part of the job. OK. This issue is not crucial anyway, so I'll commit patch 6 (+ remove markup-init.ly and its include in declarations-init.ly) as soon as I have a few minutes (which is probably not today). Nicolas
Sign in to reply to this message.
nicolas.sceaux@gmail.com writes: > On 2009/12/17 11:23:04, dak wrote: > >> Well, patch set 7 does that, but after rethinking this, I suppose I'd >> call it a mistake. Even if the option gets implemented for other >> constructs than markup, the time and memory savings are likely >> negligible. Checking for the option is likely causing more overhead >> than registering some markup. > > Another way to achieve this would have been to do something like > > (define index-markup-command > (if (ly:get-option 'make-index) > (lambda ...) ;; return code that indexes the commands > (lambda ...))) ;; return empty code > > and then call the index-markup-command function in the > define-markup-command. I've been thinking about it, also about evaluating the ly:get-option part in the macro instead of outside. However, I thought that _if_ the user wanted to index his own markups, calling ly:set-option would no longer work, and when using -dmake-index, he would automatically get Lilypond's default markups indexed as well. In short: messing with the behavior became more intricate and less predictable, at a very modest saving (if at all) of memory and performance. >> So my take would be to apply patch set 6, and possibly file a TODO >> item "just index markup, macros, whatever on documentation runs" with >> low priority that may refer to this discussion and/or patch set 7. > > OK. This issue is not crucial anyway, so I'll commit patch 6 (+ remove >markup-init.ly and its include in declarations-init.ly) Oops. I was of the opinion that I had done this already. In my git archive, it is a separate commit, but you are right that it does not yet appear in patch set 6. Sorry, I thought I had factored this better on Rietveld, but it is rather trivial. -- David Kastrup
Sign in to reply to this message.
David, These are mostly stylistic nit-picks. I'd like the section to state clearly the relationship between Lilypond scoping and the guile module system. Cheers, Ian http://codereview.appspot.com/160048/diff/13001/13002 File Documentation/contributor/programming-work.itexi (left): http://codereview.appspot.com/160048/diff/13001/13002#oldcode1271 Documentation/contributor/programming-work.itexi:1271: Maintaining this scoping when one .ly file can be included in another Is this paragraph totally irrelevant? It seems to be talking about working around an issue with scopes when one source file is \included in another. http://codereview.appspot.com/160048/diff/13001/13002#oldcode1279 Documentation/contributor/programming-work.itexi:1279: With this architecture, the guile module system is not bypassed: Retain a re-written version of this para before you go on to talk about avoiding memory leaks, maybe something like: "Scoping in Lilypond is layered on the Scheme/Guile module system." http://codereview.appspot.com/160048/diff/13001/13002 File Documentation/contributor/programming-work.itexi (right): http://codereview.appspot.com/160048/diff/13001/13002#newcode1244 Documentation/contributor/programming-work.itexi:1244: The Lilypond language has a concept of scoping, ie you can do "The Lilyond language has a concept of scoping, i.e. you can write" http://codereview.appspot.com/160048/diff/13001/13002#newcode1253 Documentation/contributor/programming-work.itexi:1253: @noindent with @code{\paper}, @code{\midi} and @code{\header} being @noindent. @code{\paper}, @code{\midi} and @code{\header} are http://codereview.appspot.com/160048/diff/13001/13002#newcode1254 Documentation/contributor/programming-work.itexi:1254: nested scope inside the @file{.ly} file-level scope. @w{@code{foo = 1}} "nested scope" --> "scope definitions nested" http://codereview.appspot.com/160048/diff/13001/13002#newcode1257 Documentation/contributor/programming-work.itexi:1257: This implemented using modules, with each scope being an anonymous "This is implemented using modules; each of these scopes is declared as an anonymous Scheme module which imports definitions from the module declared for its parent scope." http://codereview.appspot.com/160048/diff/13001/13002#newcode1258 Documentation/contributor/programming-work.itexi:1258: module that imports its enclosing scope's module. See comment for above line. http://codereview.appspot.com/160048/diff/13001/13002#newcode1261 Documentation/contributor/programming-work.itexi:1261: @code{lily} module, outside the @file{.ly} level. In the case of Lilypond's core, loaded from various @file{.scm} files, is usually defined by statements in the @code{lily} module, independently of the @file{.ly} level (top-level). In the following example http://codereview.appspot.com/160048/diff/13001/13002#newcode1268 Documentation/contributor/programming-work.itexi:1268: we want to reuse the built-in definitions, without changes effected in Here we want to re-use the built-in definitions without changes written in http://codereview.appspot.com/160048/diff/13001/13002#newcode1272 Documentation/contributor/programming-work.itexi:1272: memory leaks that could occur when running multiple files. All "memory leaks which could occur when compiling more than one file."<new line> "All"
Sign in to reply to this message.
|