Issue 2313044: T1249 - Remove (define define-ly-syntax define-public). (Closed)
|
Created:
13 years, 7 months ago by Ian Hulin (gmail) Modified:
13 years, 1 month ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionT1249 - Remove (define define-ly-syntax define-public).
This prevents a compilation error in Guile V1.9.
Replace calls to (primitive-val define-ly-syntax ...
with direct define-public declarations in the macros in this file.
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use Patrick's take 3 changes. #
Total comments: 1
Patch Set 3 : Avoid squashing T372 changes by Neil #Patch Set 4 : Revert define-syntax-simple to define-syntax for partial and squash trailing whitespace #
Total comments: 4
Patch Set 5 : Correct indentation #
Total comments: 4
Patch Set 6 : More whitespace changes #MessagesTotal messages: 14
http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (left): http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm#o... scm/ly-syntax-constructors.scm:20: (define define-ly-syntax define-public) Instead of removing this definition (like I did), would it be possible to use a macro here? Something like (defmacro define-ly-syntax (args . body) `(define-public ,args ,body)) This is untested. http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (right): http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm#n... scm/ly-syntax-constructors.scm:27: `(define-public ,args I forgot why I changed this... Is there a reason why "primitive-eval" was used here in the first place? http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm#n... scm/ly-syntax-constructors.scm:250: (define-public (partial parser location dur) This part needs to be rebased against current master.
Sign in to reply to this message.
On 2010/10/08 05:00:00, Patrick McCarty wrote: > http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm > File scm/ly-syntax-constructors.scm (left): > > http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm#o... > scm/ly-syntax-constructors.scm:20: (define define-ly-syntax define-public) > Instead of removing this definition (like I did), would it be possible to use a > macro here? Something like > > (defmacro define-ly-syntax (args . body) > `(define-public ,args ,body)) > > This is untested. > It doesn't work with Guile 1.9. It's precisely the macro parsing stuff that causes the problem. That's why I originally thought we could use (define-syntax define-ly-syntax (syntax-rules () ((_ args ... ) (define-public ... )))) This works on Guile 1.9 but fails on V1.8 when using (use-modules(ice-9 syncase)) So your 'slash and burn' solution looks the way to go. > http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm > File scm/ly-syntax-constructors.scm (right): > > http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm#n... > scm/ly-syntax-constructors.scm:27: `(define-public ,args > I forgot why I changed this... > > Is there a reason why "primitive-eval" was used here in the first place? > I don't know, but removing the cruft works with both Guile versions, and the regtests still run with Guile 1.8.7. Maybe it was a hangover from backwards compatibility with V1.6? You'll have to ask a grown-up to get the answer to this one. > http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm#n... > scm/ly-syntax-constructors.scm:250: (define-public (partial parser location dur) > This part needs to be rebased against current master. It was because I had re-based it thew up differences with your prototype. However I can see there are some potential problems here. Will investigate. Cheers, Ian
Sign in to reply to this message.
On 2010/10/09 15:31:14, ianhulin44 wrote: > On 2010/10/08 05:00:00, Patrick McCarty wrote: > > scm/ly-syntax-constructors.scm:20: (define define-ly-syntax > > define-public) > > > > Instead of removing this definition (like I did), would it be > > possible to use a macro here? Something like > > > > (defmacro define-ly-syntax (args . body) > > `(define-public ,args ,body)) > > > > This is untested. > > It doesn't work with Guile 1.9. I just tested this change with Guile 1.9, and everything checked out, though I didn't test Guile 1.8. I did a build from scratch and eliminated the cache from ~/.cache/guile just to be sure. Can you verify? I'm testing with the commit at the top of http://repo.or.cz/w/lilypond/patrick.git/shortlog/refs/heads/guile > > I forgot why I changed this... > > > > Is there a reason why "primitive-eval" was used here in the first > > place? > > I don't know, but removing the cruft works with both Guile versions, > and the regtests still run with Guile 1.8.7. Maybe it was a hangover > from backwards compatibility with V1.6? You'll have to ask a > grown-up to get the answer to this one. Okay, let's just leave this then. > > This part needs to be rebased against current master. > > It was because I had re-based it thew up differences with your > prototype. However I can see there are some potential problems here. > Will investigate. I do remember having rebase conflicts myself; this was due to one of Neil's recent commits affecting this file. Thanks, Patrick
Sign in to reply to this message.
On Sat, Oct 9, 2010 at 10:27 AM, <pnorcks@gmail.com> wrote: > > I just tested this change with Guile 1.9, and everything checked out, > though I didn't test Guile 1.8. I did a build from scratch and > eliminated the cache from ~/.cache/guile just to be sure. > > Can you verify? > > I'm testing with the commit at the top of > > http://repo.or.cz/w/lilypond/patrick.git/shortlog/refs/heads/guile I just retested and saw the compile failure that you were likely seeing. Uploaded a new "Take 3" commit. Can you test it? http://repo.or.cz/w/lilypond/patrick.git/shortlog/refs/heads/guile Thanks, Patrick
Sign in to reply to this message.
Comments actioned and tested with LilyPond using Guile V1.8.7 and V1.9.13. Ian http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (left): http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm#o... scm/ly-syntax-constructors.scm:20: (define define-ly-syntax define-public) On 2010/10/08 05:00:01, Patrick McCarty wrote: > Instead of removing this definition (like I did), would it be possible to use a > macro here? Something like > > (defmacro define-ly-syntax (args . body) > `(define-public ,args ,body)) > > This is untested. Tests out OK with V1.9 and V1.8.7 Done. http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (right): http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm#n... scm/ly-syntax-constructors.scm:27: `(define-public ,args On 2010/10/08 05:00:01, Patrick McCarty wrote: > I forgot why I changed this... > > Is there a reason why "primitive-eval" was used here in the first place? Both Guile V1.8.7 and V1.9 work without it. I assume it was cruft left over from Guile V1.6 compatibility. Ian http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm#n... scm/ly-syntax-constructors.scm:250: (define-public (partial parser location dur) On 2010/10/08 05:00:01, Patrick McCarty wrote: > This part needs to be rebased against current master. Sorted out in new patch-set Done.
Sign in to reply to this message.
Hi Ian, I found a rebasing issue that should be sorted out, as explained in my comment below. Also, I think the subject line of this patch can be improved, since we're no longer removing `define-ly-syntax', just revising it. Thanks, Patrick http://codereview.appspot.com/2313044/diff/7001/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (right): http://codereview.appspot.com/2313044/diff/7001/scm/ly-syntax-constructors.sc... scm/ly-syntax-constructors.scm:251: (define-ly-syntax-simple (partial dur) This part of the patch reverts Neil's changes as part of the fix for #372. Run the following command to see what I mean: git log -p scm/ly-syntax-constructors.scm In other words, there should be no changes to this procedure at all for your patch.
Sign in to reply to this message.
Patchset 4 uploaded partial restored to status as after T372 fix.
Sign in to reply to this message.
Greetings Ian, your patch looks very clever to me; just a possible indentation issue (I'm not sure what our policy is, but IIRC it used to be "whatever Emacs does"). BTW: This is totally unrelated to your patch, but I was recently wondering about this TODO in music-functions-init.ly, l32: "%% TODO: using define-music-function in a .scm causes crash." I'm not sure it's still relevant; is it? http://codereview.appspot.com/2313044/diff/15001/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (right): http://codereview.appspot.com/2313044/diff/15001/scm/ly-syntax-constructors.s... scm/ly-syntax-constructors.scm:20: (defmacro define-ly-syntax (args . body) Is there a way to document defmacros with docstrings instead of comments? http://codereview.appspot.com/2313044/diff/15001/scm/ly-syntax-constructors.s... scm/ly-syntax-constructors.scm:39: 'location Are you using Emacs-like indentation? "tabs for indentation, spaces for alignment"; it seems that the rest of the file is indented that way. By the way isn't there a space char missing here?
Sign in to reply to this message.
http://codereview.appspot.com/2313044/diff/15001/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (right): http://codereview.appspot.com/2313044/diff/15001/scm/ly-syntax-constructors.s... scm/ly-syntax-constructors.scm:20: (defmacro define-ly-syntax (args . body) On 2010/11/04 21:21:09, Valentin Villenave wrote: > Is there a way to document defmacros with docstrings instead of comments? I don't know. The code as is fixes the crash when compiling this stuff using Guile V1.9 and that's the aim of the patch. I suspect the whole file could be re-written using (define-syntax) and (syntax-rules), but I don't have enough scheme-fu and thinking about it made my head hurt. Ian http://codereview.appspot.com/2313044/diff/15001/scm/ly-syntax-constructors.s... scm/ly-syntax-constructors.scm:39: 'location On 2010/11/04 21:21:09, Valentin Villenave wrote: > Are you using Emacs-like indentation? "tabs for indentation, spaces for > alignment"; it seems that the rest of the file is indented that way. By the way > isn't there a space char missing here? Done.
Sign in to reply to this message.
Hi Ian, Just about there... Three lines with whitespace issues, and then everything should be ready to go. Thanks, Patrick http://codereview.appspot.com/2313044/diff/20001/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (right): http://codereview.appspot.com/2313044/diff/20001/scm/ly-syntax-constructors.s... scm/ly-syntax-constructors.scm:38: 'parser `git apply' complains about a space before the tabs on this line. http://codereview.appspot.com/2313044/diff/20001/scm/ly-syntax-constructors.s... scm/ly-syntax-constructors.scm:40: (cdr args)) For consistency, these two lines should use tabs, just like they are at present (in git).
Sign in to reply to this message.
http://codereview.appspot.com/2313044/diff/20001/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (right): http://codereview.appspot.com/2313044/diff/20001/scm/ly-syntax-constructors.s... scm/ly-syntax-constructors.scm:38: 'parser On 2010/11/04 23:50:25, Patrick McCarty wrote: > `git apply' complains about a space before the tabs on this line. Done. http://codereview.appspot.com/2313044/diff/20001/scm/ly-syntax-constructors.s... scm/ly-syntax-constructors.scm:40: (cdr args)) On 2010/11/04 23:50:25, Patrick McCarty wrote: > For consistency, these two lines should use tabs, just like they are at present > (in git). Done.
Sign in to reply to this message.
Hi Ian, LGTM. Before sending me the git patch, I would recommend changing the subject from "Remove (define define-ly-syntax define-public)" to something more accurate that reflects what this patch fixes. Thanks for your work on this! Regards, Patrick
Sign in to reply to this message.
Hi Patrick, On 10/11/10 06:01, pnorcks@gmail.com wrote: > Hi Ian, > > LGTM. > > Before sending me the git patch, I would recommend changing the subject > from "Remove (define define-ly-syntax define-public)" to something more > accurate that reflects what this patch fixes. > > Thanks for your work on this! > > Regards, > Patrick > > http://codereview.appspot.com/2313044/ Patch with amended title attached. Ian
Sign in to reply to this message.
On Wed, Nov 10, 2010 at 12:11 PM, Ian Hulin <ianhulin44@gmail.com> wrote: > > Patch with amended title attached. Thanks Ian. I've pushed your patch to git master. Regards, Patrick
Sign in to reply to this message.
|
